Change SelectedEntities to return a const ref #571
Conversation
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #571 +/- ##
==========================================
+ Coverage 77.56% 77.60% +0.04%
==========================================
Files 211 211
Lines 11579 11582 +3
==========================================
+ Hits 8981 8988 +7
+ Misses 2598 2594 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM. Could you add a note to the migration guide before merging?
Signed-off-by: Stephen Brawner <brawner@gmail.com>
| @@ -138,7 +138,7 @@ inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE { | |||
|
|
|||
| /// \brief Get the entities currently selected, in order of selection. | |||
| /// \return Vector of currently selected entities | |||
| public: std::vector<Entity> SelectedEntities() const; | |||
| public: const std::vector<Entity> &SelectedEntities() const; | |||
There was a problem hiding this comment.
@chapulina Does ignition style suggest that the & be next to the type or the method here, do you know? I just switched it to this after seeing some other getters that return references above.
There was a problem hiding this comment.
It goes next to the variable, see the style guide. This is something carried over from Gazebo classic and we have no linters to catch it yet.
| @@ -138,7 +138,7 @@ inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE { | |||
|
|
|||
| /// \brief Get the entities currently selected, in order of selection. | |||
| /// \return Vector of currently selected entities | |||
| public: std::vector<Entity> SelectedEntities() const; | |||
| public: const std::vector<Entity> &SelectedEntities() const; | |||
There was a problem hiding this comment.
It goes next to the variable, see the style guide. This is something carried over from Gazebo classic and we have no linters to catch it yet.
Instead of returning a copy of the member variable, this PR changes this getter to return a const ref. In
Scene3D.cc, a couple of places call the method without retaining ownership of the copied vector. This was specifically an issue here: https://github.com/ignitionrobotics/ign-gazebo/blob/bccea9087dd4355fc6d273d41a191d5c16563175/src/gui/plugins/scene3d/Scene3D.cc#L1846In this line, because the vector is a copy, the
back()ref operator returns a reference to an immediately deleted object, which will cause a memory read error and a subsequent segfault (in the gazebo gui).I don't think this should break API too bad. But if someone was actually mutating the copy that was returned, then they could potentially be affected by this. I don't think it can be backported, since it breaks API and ABI.
Signed-off-by: Stephen Brawner brawner@gmail.com