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

Migrate C++ command-based to use unique_ptr semantics #4303

Closed
Tracked by #4234
Starlight220 opened this issue Jun 9, 2022 · 2 comments · Fixed by #4319
Closed
Tracked by #4234

Migrate C++ command-based to use unique_ptr semantics #4303

Starlight220 opened this issue Jun 9, 2022 · 2 comments · Fixed by #4319
Labels
component: command-based WPILib Command Based Library type: discussion Questions, proposals and info that requires discussion.

Comments

@Starlight220
Copy link
Member

Starlight220 commented Jun 9, 2022

This is mostly a change in documentation/recommended usage.

The current (value) semantics of C++ command-based cause object slicing, which is difficult for both team and WPILib code. For decorators, there's an elaborate CRTP hack set up. That doesn't teams who need to specify the precise type of the command (which can change) to avoid object slicing.
Alternate options:

  • Plain pointers and references: polymorphic, but not owning.
  • Smart pointers: shared_ptr has synchronization functionality we don't need, and unique_ptr fits the ownership model and semantics we want.
    The only problem is that it uses -> instead of ., but that's what docs are for. Factories should hide the make_unique call.

The scheduler should still use plain pointers internally, and schedule/button bindings can take the underlying pointer from a unique_ptr<Command>& parameter.
Perhaps leave an opening so that teams who prefer different semantics can use them?

@Starlight220 Starlight220 added type: discussion Questions, proposals and info that requires discussion. component: command-based WPILib Command Based Library labels Jun 9, 2022
@Starlight220 Starlight220 changed the title Migrate C++ to use unique_ptr semantics. This is mostly a change in documentation/recommended usage. Migrate C++ to use unique_ptr semantics Jun 10, 2022
@Starlight220 Starlight220 changed the title Migrate C++ to use unique_ptr semantics Migrate C++ command-based to use unique_ptr semantics Jun 10, 2022
@PeterJohnson
Copy link
Member

I'm not seeing how unique_ptr can be used for commands in a way that keeps the decorator approach. The fundamental problem is the decorator functions on the class don't have access to the unique_ptr that is controlling the lifetime of the class. So there’s no way for a decorator to return the same unique_ptr that currently owns this. Using shared_ptr could work (with shared_from_this).

@Starlight220
Copy link
Member Author

After discussion, we're changing some details:
Instead of using unique_ptr directly, which would require unintuitive manipulation of std::move so we can use decorators (which don't have access to the unique_ptr owning the command), we'll be defining the decorators on CommandPtr -- a wrapper around unique_ptr that can be used with value semantics (and unlike unique_ptr, we can define methods on it).

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 type: discussion Questions, proposals and info that requires discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants