Skip to content

[cpp cmd] Unify various command types into concepts #6048

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

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

Conversation

Starlight220
Copy link
Member

Resolves #5789

@calcmogul calcmogul added the component: command-based WPILib Command Based Library label Dec 16, 2023
@Starlight220
Copy link
Member Author

I'm worried about this inflating headers and compile times due to everything becoming a template.

@KangarooKoala
Copy link
Contributor

Would an approach like the Requirements struct work? (Defining a struct with implicit conversions from everything needed?) I think that would allow the functions to remain non-templated.

@Starlight220
Copy link
Member Author

We can make the currently-explicit conversions to CommandPtr implicit...
Thoughts?

@Starlight220 Starlight220 force-pushed the cmd-concepts branch 2 times, most recently from cb5b01c to 5306525 Compare December 24, 2023 12:10
@KangarooKoala
Copy link
Contributor

It probably should be fine, especially since most teams won't need to use ToPtr() (unless for some reason they're upcasting their pointers to Command*, but that probably won't happen).

@Starlight220
Copy link
Member Author

Starlight220 commented Dec 26, 2023

I made the changes locally, and it's causing some issues. Maybe we should be consistent on everything being CommandPtr and that's it? To avoid the shenanigans of implicit conversions.

unless for some reason they're upcasting their pointers to Command*, but that probably won't happen

Wdym?

@KangarooKoala
Copy link
Contributor

unless for some reason they're upcasting their pointers to Command*, but that probably won't happen

Wdym?

I might be wrong, but as I understand it, the point of ToPtr() is so that when given a compile-time Command*, the object can correctly be converted into a CommandPtr. (Naively doing CommandPtr(std::make_unique<Command>(std::move(command)) will not properly handle the virtual methods, which I'm guessing is because it just points to the Command part of the object, without the vtable stuff for the subclass, though I might be wrong about that) However, since most users will only deal with their actual command types, CommandPtr(T&&) should use the correct type argument for make_unique, and they won't need to use ToPtr(). I could be wrong about this, though- I only figured this stuff out today.

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.

C++ Subsystem::SetDefaultCommand no longer accepts l-value references
3 participants