-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate std::bind calls to lambda expressions #4041
Migrate std::bind calls to lambda expressions #4041
Conversation
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
@tylerjw mind reviewing this one, and checking if the demos and examples need updating to match? 🧇 https://github.com/ros2/demos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I carefully read through these changes and did not find any errors. There is one small question about creating named lambdas vs using temporaries and what I think might be a small typo.
I found the PR to the examples repo but not one to the demos repo. Would you mind updating the demos repo too?
source/Tutorials/Beginner-Client-Libraries/Custom-ROS2-Interfaces.rst
Outdated
Show resolved
Hide resolved
source/Tutorials/Beginner-Client-Libraries/Writing-A-Simple-Cpp-Publisher-And-Subscriber.rst
Show resolved
Hide resolved
Co-authored-by: Tyler Weaver <maybe@tylerjw.dev> Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
@tylerjw thank you for your review!
I can also create a PR to update the demos repo, sounds reasonable to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FelipeGdM thanks for your contribution.
i have a quick question.
and
are excluded intentionally? i guess. since those are examples for Webots?
@fujitatomoya they were not excluded intentionally, it was an oversight on my part 🤦 I will include them in the migration |
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the latest changes and they look good. Thank you for the PR to the demos repo.
source/Tutorials/Beginner-Client-Libraries/Writing-A-Simple-Cpp-Publisher-And-Subscriber.rst
Show resolved
Hide resolved
@clalancette I'm sorry, I didn't understand the issue. As far as I understood, the PR ros2/examples#317 adds examples for services using lambdas, but the file you mentioned is about topics. I can see that the PR performs some cosmetic changes in the topics lambda example, such as adding comments here and there, but the code is essentially the same. I may be missing something in the relationship between the docs and the examples, if that's the case it would be fine to remove this change and create a draft PR with it and merge once the other one is closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No, you are right. When I looked at it, I got confused. This part of it is OK, I'll resolve that conversation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more updates (mostly to the geometry_tutorials), then I think this will be good to go.
source/Tutorials/Intermediate/Writing-an-Action-Server-Client/Cpp.rst
Outdated
Show resolved
Hide resolved
…Cpp.rst Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Felipe Gomes de Melo <felipegmelo.42@gmail.com>
@clalancette I just created the PR in the geometry_tutorials repo and resolved the conversations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all of the work to push this over the line. I suspect that there will be small followups in the next couple of weeks, but this looks good to me so I'll go ahead and merge it.
Great to see this in! |
As discussed in this issue, most of the examples in the docs were written using the C++11 function
std::bind
, which is similar to the ROS 1 way of doing things withboost::bind
. In later revisions of C++, lambda expressions became more powerful and today are consider to be more "idiomatic" thanstd::bind
This PR migrates the C++ examples that use
std::bind
to equivalent lambda expression codeThe affected examples are:
In the original issue, it's said that it would be interessant to keep some exemples with
std::bind
as a way to show that it's a possible way to do things in C++. I personally don't know which criteria would be better to choose which example to keep usingstd::bind
, so a listed them all above so other people may say what they think 😁Closes #3618