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

C++ Sendable Chooser template arguement requirements #5940

Closed
ncorrea210 opened this issue Nov 20, 2023 · 9 comments
Closed

C++ Sendable Chooser template arguement requirements #5940

ncorrea210 opened this issue Nov 20, 2023 · 9 comments

Comments

@ncorrea210
Copy link
Contributor

ncorrea210 commented Nov 20, 2023

Describe the question you have.
Why does the sendable chooser require the template arguement to be copy constructible?

Describe the reason for your confusion.
In the implementation of sendable chooser it only uses the move constructor so it seems odd that it is required to be copy constructible.

Is your question related to a problem? Please describe.
I'd like to be able to use frc2::CommandPtr in the sendable chooser which is not copy constructible but it is move constructible. Would it be possible to require the template arguement to be std::is_move_constructible or std::is_nothrow_move_constructible instead of requiring it to be copy constructible?

@Starlight220
Copy link
Member

Starlight220 commented Nov 20, 2023

In the implementation of sendable chooser it only uses the move constructor

Can you back this? Last time I checked this wasn't the case; it also doesn't logically make sense with the idea of SendableChooser (which should be reusable - which can't happen if the objects are moved out).

See my comment here.

@ncorrea210
Copy link
Contributor Author

I've copied a piece of code from SmartDashboard.inc below:

template <class T>
  requires std::copy_constructible<T> && std::default_initializable<T>
void SendableChooser<T>::AddOption(std::string_view name, T object) {
  m_choices[name] = std::move(object);
}

I may be mistaken but I think this will call the move assignment operator which then calls the move constructor doesn't it? My mistake If I have read this wrong.

@Starlight220
Copy link
Member

Starlight220 commented Nov 20, 2023

The options are moved into the chooser (according to the snippet you've sent), but they're copied out (see GetSelected) -- otherwise you'd have invalid objects at the second call.

@ncorrea210
Copy link
Contributor Author

ncorrea210 commented Nov 21, 2023

I guess I'm wondering then why the Sendable Chooser does not have CommandPtr in mind then? If the goal is to make C++ programming safer it seems like encouraging the use of CommandPtr over Command* would be good however CommandPtr is not and cannot be compatible with Sendable Chooser in its current states as a unique_ptr wrapper. I think some of this is mentioned in the issue you referenced before, #5921.

@Starlight220
Copy link
Member

I commented on that issue with solutions, here are the solutions I mentioned there:

  • Choose between Command*, own the commands externally in RobotContainer
  • Choose between function<CommandPtr()>.
  • Choose between strings/ints/enums/etc, feed them into SelectCommand.

@PeterJohnson
Copy link
Member

We can change GetSelected to return a const(?) ref instead of a copy, but I’m not sure that’s really all that useful of a change, as the caller would probably need to copy it anyway?

@Starlight220
Copy link
Member

A const ref of CommandPtr won't help.

@ncorrea210
Copy link
Contributor Author

If the CommandPtr wrapped a shared pointer than it could be made to be copy constructable. Not sure if it defeats the purpose of CommandPtr but that could make it work for SendableChooser.

@Starlight220
Copy link
Member

That would defeat the point of CommandPtr.

I've already illustrated three methods for doing this. You are right in that docs/examples should use these solutions (and the possible addition of a ChooserCommand), but that's already in the realm of #5921 and not in the scope of this issue.

I don't see any action items left here.

@Starlight220 Starlight220 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants