Subscription callbacks (clang-tidy vs. const-ref)

[henningkayser]

With clang-tidy-fix enabled we now can't use const shared pointers to const in subscription callbacks anymore (see https://travis-ci.com/github/ros-planning/moveit2/jobs/347903960#L809). Unfortunately, const references are not supported (ros2/rclcpp#281) so for now we basically have to pick one of the following options.

1. Disable linting for all callback functions and use callback(const Message::ConstSharedPtr msg)
2. callback(const Message::SharedPtr msg) errors because of const
3. callback(Message::ConstSharedPtr msg) errors because of const

I would opt for 2 since it's a rule that fits for all (also service callbacks and is closest to ROS 1 codebase) even though it's not really nice to have a non-const message access everywhere.

Update

So, apparently we need to disable clang-tidy rules to make this work. Do we want to do this on a case-by-case basis with // NOLINTNEXTLINE(performance-unnecessary-value-param) or project-wide? I would prefer keeping the rule enabled and keep an issue open to remove the lint ignores once const-ref callbacks are available. The only alternative we still have is to use fully non-const shared pointers, but I don't think we want to go that route.

[v4hn]

Why do you not consider

4. Do everyone a favor and provide a patch for rclcpp to support const& and apply (1) (or disable the resp. clang-tidy check entirely) until the patch is upstream.

If you throw away Const, that might affect performance (I don't know enough about the internals of rclcpp to be sure) because you need a modifiable copy of the message which cannot be shared with other callbacks. 3. would be the better option because it just copies the shared ptr.

I feel I'm repeating myself a lot: From my perspective porting MoveIt should show and address possible shortcomings in the lower layers of ROS2 as well. I'm not sure whether your grant disagrees, but I'm convinced @gavanderhoorn agrees with my point of view.

[henningkayser]

Why do you not consider

1. Do everyone a favor and provide a patch for rclcpp to support const& and apply (1)

That's considered, but not an option that solves the issue now.

(or disable the resp. clang-tidy check entirely) until the patch is upstream.

Sure, if we know we can get that patch upstream soon. Otherwise, I would prefer just ignoring the check per case as in 1.

If you throw away Const, that might affect performance (I don't know enough about the internals of rclcpp to be sure) because you need a modifiable copy of the message which cannot be shared with other callbacks.

That's right, but as it turns out 2 and 3 are not valid options anyway as it also errors for the same reason. "Disabling the rule" is the winner.

3. would be the better option because it just copies the shared ptr.

I feel I'm repeating myself a lot: From my perspective porting MoveIt should show and address possible shortcomings in the lower layers of ROS2 as well. I'm not sure whether your grant disagrees, but I'm convinced @gavanderhoorn agrees with my point of view.

Sure, I do as well

[v4hn]

but not an option that solves the issue now.

It does, even if you need an ugly NOLINT hack or a forked version of an upstream dependency to bridge a transition period. If you consider upstream contributions not to solve problems (the "but we need it NOW" is true for all problems), there would not be any sense to contribute at all.

Sure, if we know we can get that patch upstream soon

Well the reason it's not upstream yet (though reported years ago) is mostly because nobody wrote the patch...

[ChrisThrasher]

ros2/rclcpp#281 has been closed. Can we revisit this now that the problem has seemingly been fixed in rclcpp?

[henningkayser]

ros2/rclcpp#281 has been closed. Can we revisit this now that the problem has seemingly been fixed in rclcpp?

I think we should. PRs are always welcome ;)

[henningkayser]

I think this has been fixed with #1706