[cpp cmd] Add Requirements struct

[KangarooKoala]

Fixes #4793

Potential changes to user code:

• Because AddRequirements({subsystemPointer}) now selects the Subsystem* overload instead of the initializer list (now Requirements struct) overload, it causes warnings about the redundant braces. Adding an overload taking std::initializer_list (which could defer to the Requirements overload) would fix this issue, but I don't know how important that use case is.
• Additionally, any code using implicit conversions from a type to std::initializer_list or std::span would also need to be written so at least one of those conversions is explicit. (For example, AddRequirements(array) no longer works, but AddRequirements(Requirements{array}) or AddRequirements({array}) do.)

Notes:

• The parameters don't need to keep prevent duplicates, so we can use std::vector. However, Mac 12 doesn't seem to support the constexpr std::vector constructors (see https://discord.com/channels/176186766946992128/1137101704127578236), so it's left non-constexpr for now. If there's a fix, though, we could always implement that.
• Currently, every copy of the Requirements struct also copies the vector, which I'm pretty sure copies the pointers to subsystems. This could lead to a lot of copies in nested calls, such as cmd::RunOnce -> InstantCommand -> FunctionalCommand -> AddRequirements. This isn't necessarily expensive, but we *could* avoid them by making the backing structure a span, at the cost of leading to bugs if the struct is initialized from an initializer list and we use the struct outside of the function call (e.g., store it in an instance variable which we access later).
• This currently uses implicit conversions, which are documented in the Requirements struct and AddRequirements, but the conversions can be made explicit if desired (or more documentation could be added).
• This kind of pattern (struct taking initializer list and span overloads) could be useful elsewhere- e.g., CommandScheduler::RegisterSubsystems, CommandScheduler::Cancel, and potentially other parts of wpilib- but I think changing those usages would be out of the scope of this PR.

[Starlight220]

Can you add tests confirming that all the semantics and resolved overloads we intend are indeed what happens? With implicit conversions and overloads it's always better to be sure.

Additionally, an idea (open for discussion) -- maybe we can deprecate/remove the AddRequirements(Subsystem*) overload? Adding the braces isn't significantly verbose, and if it complicates overload resolution than maybe we don't need it. How much benefit does it provide?

[KangarooKoala]

Additionally, an idea (open for discussion) -- maybe we can deprecate/remove the AddRequirements(Subsystem*) overload? Adding the braces isn't significantly verbose, and if it complicates overload resolution than maybe we don't need it. How much benefit does it provide?

The only benefit I can think of is that conceptually it's nice when teams define their own command classes (instead of using the factory methods), since those command classes usually only require one subsystem. I haven't seen too many teams' robot code, so I might be missing other benefits, but if that is the only benefit, I'd feel fine removing it since I don't think anything else provides Subsystem* overload.

Another thought I just had is that we could rename AddRequirements(Subsystem*) to AddRequirement(Subsystem*), though at that point I'm not sure what benefits it has over just removing the overload entirely.

[KangarooKoala]

I wasn't sure if I should add a test for the AddRequirements({&subsystem}) overload resolution behavior, since I'm not sure we want to enforce that behavior. Additionally, there isn't a SmallSetDuplicatesSemantics test because I didn't think it was necessary because the argument itself guarantees no duplicates. If desired, though, I could add it for consistency.

[Starlight220]

Let's deprecate the AddRequirements(Subsystem*) overload. (Unless we can remove it directly and have a project importer regex -- @PeterJohnson ?)

Also, is there a reason to keep the SmallSet overload?

[KangarooKoala]

I wasn't completely sure about what to recommend teams use instead of the deprecated overload, so any thoughts would be appreciated.

Also, is there a reason to keep the SmallSet overload?

We use the SmallSet overload in the command groups when they add the requirements of their component commands.

[Starlight220]

I wasn't completely sure about what to recommend teams use instead of the deprecated overload, so any thoughts would be appreciated.

Oh, right -- {&requirement} doesn't work. I don't know, but {&requirement, &requirement} feels like a hacky workaround we shouldn't be recommending. Maybe keep the initializer_list for a year, and then outright remove it next year (thanks to the implicit conversion)?
Or maybe just keep the Subsystem* overload?

@PeterJohnson -- your thoughts?

[rzblue]

I'm in favor of just keeping the Subsystem* overload.

[Starlight220]

So let's just do that.

[rzblue]

@KangarooKoala This needs to be rebased on main- merging with main caused a bunch of files to be touched

[KangarooKoala]

Seems like clang 14 doesn't recognize std::vector default initialization, since only the sanitizers are failing and they specify clang 14 in the configure step. Not sure what the best solution is- We could just put the braces back, but maybe there's a better solution?

[calcmogul]

I think it's because the member variable is const-qualified. Making member variables const-qualified is generally a bad idea because it breaks things like copy construction and copy assignment. Enforce immutability through the interface instead. The iterator functions are already const-qualified, so you should be able to just remove the const from the member variable.

[Starlight220]

If you think MockAddRequirementsCommand shouldn't inherit from Command, go for it.

If not, I think this is good to go.