Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[commands] Mark CommandPtr class as [[nodiscard]] #7803

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bathtubed
Copy link

Classes can be marked as [[nodiscard]] such that if they are ever returned from a method its as if that method has the [[nodiscard]] attribute. (https://en.cppreference.com/w/cpp/language/attributes/nodiscard)

Seeing as all the CommandPtr decorators were marked as [[nodiscard]], I thought this would achieve the goal more succinctly.
This has the added benefit of also making user created CommandPtr factories benefit as well.

What motivated this is that I've seen student code like the following several times

CommandPtr ActivateIntake() {
  return RunEnd(
    [this] {SetIntakeSpeed(0.5);},
    [this] {SetIntakeSpeed(0.0);});
}

CommandPtr IntakeCoral() {
  return Run([this] {ActivateIntake();})
    .Until([this] {return CoralDetected();});
}

This code currently happily compiles without warnings and looks correct to someone unfamiliar with lambdas, async programming, or wpilib command semantics. (It says Run ActivateIntake until CoralDetected, sounds good to me!)
Essentially, students often get confused between methods which run "instantly" and methods which return commands.

This change makes it a warning, (which can be made an error with -Werror as I've set up our project to do and I recommend to everyone, especially with new programmers).

…f each decorator

This has the same effect but makes it so any user code returning CommandPtr can't discard a returned command

Signed-off-by: Eric Ward <[email protected]>
@bathtubed bathtubed requested a review from a team as a code owner February 19, 2025 06:25
@github-actions github-actions bot added the component: command-based WPILib Command Based Library label Feb 19, 2025
@PeterJohnson PeterJohnson changed the title [wpilibc, commands] marks CommandPtr class as [[nodiscard]] [commands] Mark CommandPtr class as [[nodiscard]] Feb 19, 2025
…in a variable rather than discarding

Signed-off-by: Eric Ward <[email protected]>
@bathtubed
Copy link
Author

/format

@Gold856
Copy link
Contributor

Gold856 commented Feb 19, 2025

The format command is temporarily disabled. You can either run the formatter locally, or download the patch from the Lint and Format job here: https://github.com/wpilibsuite/allwpilib/actions/runs/13407671136?pr=7803

Signed-off-by: Eric Ward <[email protected]>
@@ -25,7 +25,8 @@ namespace frc2 {
* std::unique_ptr<Command>, use CommandPtr::Unwrap to convert.
* CommandPtr::UnwrapVector does the same for vectors.
*/
class CommandPtr final {
class [[nodiscard]]
CommandPtr final {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wpilibsuite/styleguide are we expecting the C++ formatter to line break here? Seems like a rather odd point to break the line to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found it strange, perhaps it expects line breaks after attributes in declarations universally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I recall the C++ formatter can be a bit weird with attributes, but unfortunately I don't remember where I encountered this before and what we did to fix it.

Copy link
Member

@calcmogul calcmogul Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we have BreakAfterAttributes: Always set in .clang-format. The options are "always", "leave as-is", and "never": https://clang.llvm.org/docs/ClangFormatStyleOptions.html#breakafterattributes

We set this because otherwise long [[deprecated("msg")]] attributes make certain function declarations very ugly.

@KangarooKoala
Copy link
Contributor

Love to see this! While we're at it, it'd be cool to remove the unnecessary [[nodiscard]] from Commands.h and Subsystem.h as well. (The Subsystem command factories are defined at the bottom of the class, the first one is RunOnce().)

@bathtubed
Copy link
Author

Love to see this! While we're at it, it'd be cool to remove the unnecessary [[nodiscard]] from Commands.h and Subsystem.h as well. (The Subsystem command factories are defined at the bottom of the class, the first one is RunOnce().)

Done, not super familiar with the codebase and don't have the tooling set up to search for any other spots that might be returning CommandPtrs so not sure if I missed other files.

@KangarooKoala
Copy link
Contributor

Did a grep -n nodiscard $(find */src/* -type f) and here's what I found (after some processing):

wpilibNewCommands/src/main/native/include/frc2/command/Command.h:195
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:206
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:217
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:227
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:238
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:248
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:260
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:269
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:285
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:297
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:309
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:333
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:344
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:355
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:364
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:373
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:385
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:397
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:407
wpilibNewCommands/src/main/native/include/frc2/command/Command.h:416
wpilibcExamples/src/main/cpp/examples/RapidReactCommandBot/include/subsystems/Drive.h:42
wpilibcExamples/src/main/cpp/examples/RapidReactCommandBot/include/subsystems/Drive.h:51
wpilibcExamples/src/main/cpp/examples/RapidReactCommandBot/include/subsystems/Shooter.h:31
wpilibcExamples/src/main/cpp/examples/RapidReactCommandBot/include/subsystems/Intake.h:22
wpilibcExamples/src/main/cpp/examples/RapidReactCommandBot/include/subsystems/Intake.h:26
wpilibcExamples/src/main/cpp/examples/RapidReactCommandBot/include/subsystems/Storage.h:19
wpilibcExamples/src/main/cpp/examples/RapidReactCommandBot/include/subsystems/Pneumatics.h:20
wpilibcExamples/src/main/cpp/examples/DriveDistanceOffboard/include/subsystems/DriveSubsystem.h:85
wpilibcExamples/src/main/cpp/examples/DriveDistanceOffboard/include/subsystems/DriveSubsystem.h:95

There were a lot of other results that I didn't manually verify weren't for CommandPtr, but I'm 95% certain that those other spots wouldn't return CommandPtr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: command-based WPILib Command Based Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants