-
Notifications
You must be signed in to change notification settings - Fork 611
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
Comments
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. |
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. |
The options are moved into the chooser (according to the snippet you've sent), but they're copied out (see |
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. |
I commented on that issue with solutions, here are the solutions I mentioned there:
|
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? |
A const ref of CommandPtr won't help. |
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. |
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. |
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?
The text was updated successfully, but these errors were encountered: