Skip to content
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

Use boost/bind/bind.hpp to fix warnings on Arch Linux #3156

Merged
merged 5 commits into from
Jan 12, 2022

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Jan 8, 2022

It was previously reported in #2757 that there were numerous compiler warnings complaining about using boost bind placeholders (like _1) in the global namespace, since that practice is deprecated. This was fixed in #2809 for purposes of our CI machines, but as noted in #3147 it is still an issue for some users.

This takes an alternative approach to #3147 by avoiding use of the boost/bind.hpp header file in favor of boost/bind/bind.hpp, as recommended in boost/bind.hpp itself. This worked directly for all files except gazebo/gui/building/EditorView.cc, which had some arcane compiler errors. Since it was only one file; I migrated it to use std::bind with std::placeholders::_1 in 4614ef9.

cc @alexdewar

The boost/bind.hpp file recommends switching to
boost/bind/bind.hpp to avoid a legacy syntax
for placeholders.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
There was a compilation issue with boost/bind/bind.hpp
in EditorView.cc, and it was easy to fix it by
switching to std::bind with std::placeholders::_1.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Use narrow scoping when possible, except in
model_database.cc integration test.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@alexdewar
Copy link
Contributor

Looks good to me -- and it's a more sensible fix than my hacky workaround (PR #3147). I tried build-testing it and I think the warnings were fixed although compilation ultimately failed because of #2867. Given the nature of the change though, I think as long as it passes your own CI checks then it should be fine.

My one suggestion is that you might consider using either the boost version of bind or std::bind throughout, for consistency.

(And FYI I've been testing things on Arch Linux with boost v1.78.0.)

@scpeters
Copy link
Member Author

My one suggestion is that you might consider using either the boost version of bind or std::bind throughout, for consistency.

I agree it would be nice to be consistent with std::bind and boost::bind. At one point we were working to "un-boost" gazebo, replacing boost::bind with std::bind since they have the same API (as far as I have seen), but this hasn't been a priority recently. I would be happy to review a pull request that does this migration for some or all of the codebase.

@alexdewar
Copy link
Contributor

Fair enough. I can see that that wouldn't be a priority.

Thanks for tackling my issue!

@scpeters
Copy link
Member Author

I just noticed that a few files were including boost/bind.hpp but not calling boost::bind, so I've removed the include statements in 4c18c24

@scpeters scpeters changed the title Fix boost placeholder warnings Use boost/bind/bind.hpp to fix warnings on Arch Linux Jan 11, 2022
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@scpeters scpeters merged commit 897f6cb into gazebosim:gazebo11 Jan 12, 2022
@scpeters scpeters deleted the boost_placeholder_warnings branch January 12, 2022 01:10
@traversaro
Copy link
Collaborator

traversaro commented Jan 13, 2022

This is probably expected and nothing can be done for it, but note that this could create several downstream compilation errors. I got robotology/gazebo-yarp-plugins#606, but I guess similar errors will appear in gazebo_ros_pkgs (see https://github.com/ros-simulation/gazebo_ros_pkgs/search?q=_1).

fyi @scpeters @j-rivero

@scpeters
Copy link
Member Author

This is probably expected and nothing can be done for it, but note that this could create several downstream compilation errors. I got robotology/gazebo-yarp-plugins#606, but I guess similar errors will appear in gazebo_ros_pkgs (see https://github.com/ros-simulation/gazebo_ros_pkgs/search?q=_1).

fyi @scpeters @j-rivero

I hadn't thought about that aspect of it. I may revert this...

@alexdewar
Copy link
Contributor

Apologies, I only just saw this. It's annoying that the change breaks downstream code which relies on the global namespace being polluted with _1 etc. That's a bit gross and the boost folks were right to deprecate it, but obviously we can't break the API for a minor version bump. Maybe something to consider for the next API-breaking version change though?

@scpeters
Copy link
Member Author

Apologies, I only just saw this. It's annoying that the change breaks downstream code which relies on the global namespace being polluted with _1 etc. That's a bit gross and the boost folks were right to deprecate it, but obviously we can't break the API for a minor version bump. Maybe something to consider for the next API-breaking version change though?

we aren't planning a gazebo12 release, but if we do then this would be a reasonable change to make

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants