Improve move_group_interface const correctness #1715
Conversation
logical state of the class. Added noexcept modifiers to move constructors, which should always be marked as noexcept.
iterators so that const iterators are compared with const iterators.
|
Thanks for helping in improving MoveIt |
jliukkonen
changed the title
jliukkonen
changed the title
jliukkonen
changed the title
There was a problem hiding this comment.
Thank you a lot for your work here. This is important details.
Please have a look at my inline comments.
@maintainers: It's not clear to me whether we want to define interactions with the ROS system, especially creating new subscribers and calling actions as const.
Personally, I'm not a big fan of allowing const action clients, but opinions probably differ.
...ng_interface/move_group_interface/include/moveit/move_group_interface/move_group_interface.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Please remove the changes to auto iterators. We've had this debate before as a community and have documented the decision in our style guide, let's not rehash that again:
Note that modernize-loop-convert check may convert for (...; ...; ...) loops to for (auto & ... : ...). However, auto is not an expression highly readable. Please explicitly specify the variable type, if it doesn’t become clear immediately from the context:
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
Outdated
Show resolved
Hide resolved
|
@jliukkonen, thanks for addressing the remaining comments. Please also fix the clang-tidy issues on Travis. The |
|
Big thank you to the reviewers for taking their time commenting and proposing changes. It seems that right now the only(?) thing to fix are those clang-tidy issues. @rhaschke is there a way to have more detailed report on what is failing? I wasn't expecting to see clang-tidy issues and I don't know right now what is causing the check to fail. As for the catkin-lint issue, do you mean by unrelated to ignore it or to fix it while understanding that it is unrelated to problems that make clang-tidy check fail? |
|
Travis reports issues in |
in getNamedTargets. Switch to range based for-loops to pass clang-tidy tests.
jliukkonen
force-pushed
the
move_group-improve-const-correctness
branch
from
a166d0e
to
463a4b1
Compare
|
Congrats on getting your first MoveIt pull request merged and improving open source robotics! |
Improved const correctness in
move_group_interfaceby addingconstqualifier to member functions which do not modify the logical state. This PR also fixes couple instances where non-const iterators were compared against const iterators.The interface defines move constructors which are now marked as noexcept. Long iterator types have been replaced with an auto type to improve readability of the code.