-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Explicit assignment operators for Observation, default assignment is a bug here #2413
Conversation
Also move constructor. Default copy assignment is not just a shortcut to copy constructor, sadly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I don't see the value in this PR. What are you trying to accomplish that was not possible prior, specifically?
I guess there's an alternative to those verbose constructors/assignments. I didn't try to compile it, but it looks like EDIT: Checked. It would still require this: Observation & operator=(const Observation & obs){
origin_ = obs.origin_;
cloud_.reset(new sensor_msgs::msg::PointCloud2(*(obs.cloud_)));
obstacle_range_ = obs.obstacle_range_;
raytrace_range_ = obs.raytrace_range_;
return *this;
} But move functions could easily be defined as: Observation(Observation && obs) = default;
Observation & operator=(Observation && obs) = default; |
Yes, a unique pointer would also work. I'd accept a PR to do that if you like. But please do remove the other unnecessary changes and lets just focus on the copy operator that isn't handling the PC2 correctly. This PR also has linting issues, please make sure to run the test suite, especially |
Not sure what exactly should I do, so for now just removed functions unrelated to the bug and fixed linting issues. If I should replace the pointer with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please submit against Galactic and Main branches and I can merge the full set of 3 together
If you wished to switch to a unique pointer as well, that would be fine, but not required. I'm OK with it either way. It can be included in this PR not a new one.
You will also need to submit a PR https://github.com/ros-planning/navigation.ros.org/blob/master/migration/Galactic.rst against this file adding a new section about this change to the migration guide.
OK, two followup PRs are done: #2425 #2424
But what to submit there? This is just a bugfix, and there's no difference between I can think of only
or
to "Major Improvements", but that's not exactly major. |
Also move constructor. Default copy assignment is not just a shortcut to copy constructor, sadly
Basic Info
Description of contribution in a few bullet points
new
call forsensor_msgs::msg::PointCloud2 * cloud_
std::vector
with less (de)allocationsDescription of documentation updates required from your changes
Teeeny changes:
Future work that may be required in bullet points
crystal
andmain
braches should be doneFor Maintainers: