Use of std::bind in C++ code examples

[FelipeGdM]

Hi everyone!

In the tutorial page Writing a simple publisher and subscriber (C++), the code example provided for the MinimalPublisher uses the C++11 function std::bind at the wall timer creation

timer_ = this->create_wall_timer(
      500ms, std::bind(&MinimalPublisher::timer_callback, this));

In C++14 and later, lambda expressions are found to be preferred as they are easier to write and understand than std::bind. Static analyzers such as clang-tidy and SonarSource consider the use of std::bind as a code smell in C++14 and newer versions

As a personal comment, I found this line of code to be the most difficult to explain to ROS 2 newcomers, as usually students tend to learn ROS 2 at the same time they are learning modern C++. Any improvement of the readability of the Beginner Tutorials may be a huge benefit when teaching ROS 2 to newbies

The ROS 2 cpp examples folder already have a publisher example using lambda expressions, so my question is, is there any reason to keep the std::bind version in the docs page?

If the answer is no, I would gladly update it to use a more readable and modern idiom of C++

Kind regards,
Felipe Gomes de Melo

[clalancette]

The ROS 2 cpp examples folder already have a publisher example using lambda expressions, so my question is, is there any reason to keep the std::bind version in the docs page?

While I know that std::bind is somewhat considered deprecated, I do think that there are situations in which it is easier to read. It is also something that the core code supports. So I do think that we should continue to have examples where we use it.

Whether we should change the default beginner example to use lambdas I'm more ambivalent about. I don't feel like lambdas are substantially easier to explain than std::bind, but maybe others have a different opinion. @ros2/team, any thoughts?

[mjcarroll]

While I know that std::bind is somewhat considered deprecated, I do think that there are situations in which it is easier to read. It is also something that the core code supports. So I do think that we should continue to have examples where we use it.

It's also a very common pattern in ROS 1 code, as boost::bind was the de facto way of doing this. I think preferring the modern idiom is important, but also showing a clear migration path with familiar concepts is helpful.

[codebot]

This is tricky. C++ is just a difficult language since there are 99 ways of doing everything, most of which will happily compile and then sometimes result in undefined behavior 💥 🤦 That being said, I agree it's most programmer-friendly to use mainstream practices wherever we can. I'm in favor of having the default entry docs use lambdas since these days it's the most common practice. A lot has changed in the C++ world since these examples were first written.

I do think it's tough to learn lambdas at first, because the syntax is all just punctuation without any tokens that you can type into a search engine to find help. But, hopefully many/most people using the API will already be familiar with lambdas from other contexts in modern C++ since they are now so common. I'd be in favor of keeping the std::bind() approach around the docs/examples somewhere (in a ROS 1 migration guide section, maybe?) with some code-comments explaining that this still works, but is no longer the default approach.

Thank you for looking into this and improving the documentation! 🤩

[clalancette]

We discussed this more, and the general feeling is that we should move more of our examples, particularly the default examples, to use lambdas. That said, we should still keep around examples (and some documentation) that uses std::bind, because that is a feature that is supported by the core and it makes it easier for people porting from ROS 1.

So with that said, @FelipeGdM we'd welcome changes to the documentation that made this update. Thanks!