Do not pass and return simple types by const ref #859

[Nils-ChristianIseke]

Description

Removed const references of simple types with regex.

Checklist

• [x ] Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[codecov]

Codecov Report

Patch coverage: 70.00% and project coverage change: +0.01% 🎉

Comparison is base (6482350) 50.73% compared to head (2b9c72a) 50.74%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2119      +/-   ##
==========================================
+ Coverage   50.73%   50.74%   +0.01%     
==========================================
  Files         386      386              
  Lines       31970    31970              
==========================================
+ Hits        16216    16219       +3     
+ Misses      15754    15751       -3     

Files Changed	Coverage Δ
...llision_detection/src/collision_octomap_filter.cpp	0.00% <0.00%> (ø)
...include/moveit/robot_trajectory/robot_trajectory.h	98.67% <ø> (ø)
...ustrial_motion_planner/trajectory_generator_circ.h	100.00% <ø> (ø)
...dustrial_motion_planner/trajectory_generator_lin.h	100.00% <ø> (ø)
...dustrial_motion_planner/trajectory_generator_ptp.h	100.00% <ø> (ø)
...strial_motion_planner/src/trajectory_functions.cpp	100.00% <ø> (ø)
...l_motion_planner/src/trajectory_generator_circ.cpp	99.09% <ø> (ø)
...al_motion_planner/src/trajectory_generator_lin.cpp	94.45% <ø> (ø)
...al_motion_planner/src/trajectory_generator_ptp.cpp	98.92% <ø> (ø)
..._detection_bullet/bullet_integration/basic_types.h	100.00% <100.00%> (ø)
... and 4 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[henningkayser]

CI fails for unrelated formatting reasons /~https://github.com/ros-planning/moveit2/actions/runs/4862228141/jobs/8670864807?pr=2119#step:12:9677. I'm fine with these changes in general. @tylerjw what do you think?

[tylerjw]

I'm also fine with these changes. I do think having const in parameters where it is meaningless should also be removed.

Here is what I mean. Take this function you want to improve:

void do_magic(const int& input_number);

If you change it to this (just remove the &) you leave const which is meaningless in modifying the int. Previously it modified the way the & worked but that is no longer there:

void do_magic(const int input_number);

A much better change in my opinion is to also remove the const keyword in cases where we took a const& and made it a value parameter like this:

void do_magic(int input_number);

[Nils-ChristianIseke]

Thanks for the suggestions @tylerjw. What are the advantages of not using const? Is it really bad to use a const, if it is not necessary?

[gavanderhoorn]

Without the reference, the function is getting a copy. The rationale is that if you're passing a copy already, const doesn't really make any difference any more, as the "original" can't be modified anyway.

The function/method could still change its argument, but that would not affect the variable/value that was passed to it.

Hence no need for const.

(this is not the only opinion about this btw. Some developers consider the const to also say something about the semantics of the function, as in: this argument is used in a read-only manner only. I don't have any opinion, but did want to point out that it's not necessarily the case there's full consensus about these kinds of things)

[tylerjw]

More details than you probably asked for:

Because of the C model of separating declaration (the type found in headers) and definition (the details found in the source file) the keyword const for value parameters (not pointers) has both meaning and no meaning.

There is nothing in the way C or C++ encodes const in the type of the function for non-pointer (value) parameters. For this reason if you use const on a value parameter in a header file the compiler just silently ignores it. The reason it doesn't throw an error telling you it is meaningless is probably some historical accident.

At the same time you can put const on your value parameters in the definition and it does have an affect. The affect when you put it there is that it makes it const in the same way you can make local variables const. You will then get compile errors if you try to change the value in the scope of the function. Because of the previous detail that const is meaningless in the type definitions that include const value parameters match just fine with declarations that omit the const value parameters.

I am a fan of local const variables but 🤷. I think C++ is already a language full of noise so we should avoid adding keywords that have no symantic meaning and are ignored by the compiler.

[Nils-ChristianIseke]

Thanks @gavanderhoorn and @tylerjw for the detailled explanation, that was exactly what i was looking for. If it is consensus to remove the const keyword, i can make the change as soon as i find the time (hopefully within 1 week).

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[tylerjw]

Thanks @gavanderhoorn and @tylerjw for the detailled explanation, that was exactly what i was looking for. If it is consensus to remove the const keyword, i can make the change as soon as i find the time (hopefully within 1 week).

Yes, please remove const where it is meaningless (value parameters).

[Shobuj-Paul]

The clang-tidy warnings in CI will be fixed by #2343
This PR can be rebased and merged right after.

[mergify]

This pull request is in conflict. Could you fix it @Nils-ChristianIseke?

[Shobuj-Paul]

@sjahr @tylerjw
If no further work is being done on this PR, should I take over and finish it according to the above recommendations?

[tylerjw]

@Shobuj-Paul you are welcome to take over this work and finish it. Ping me for reviews.

[mergify]

This pull request is in conflict. Could you fix it @Nils-ChristianIseke?

[sjahr]

Merged with #2453

[Nils-ChristianIseke]

Thanks for catching up on this issue @Shobuj-Paul.