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 default CXX, use boost::placeholders::_1 instead of _1 #750

Merged
merged 1 commit into from
May 24, 2022

Conversation

lucasw
Copy link

@lucasw lucasw commented May 5, 2022

I'm curious if std::bind with std::placeholders also works (and will likely try it), but here I kept using boost for a smaller change.

@lucasw lucasw changed the title Use default CXX, specify boost::placeholders::_1 Use default CXX, use boost::placeholders::_1 instead of _1 May 5, 2022
@SteveMacenski
Copy link
Collaborator

Is there any benefit to this? Mixing and matching doesn't seem like a great policy to maintain

@lucasw lucasw force-pushed the boost_placeholders_cpp17 branch from eb4493c to 9a95c78 Compare May 5, 2022 17:26
@lucasw
Copy link
Author

lucasw commented May 5, 2022

This PR as-is doesn't involve any mixing and matching or changeover to std::bind and is purely boost::bind, I'm only wondering about std::bind vs. boost::bind in my comment above but not proposing it here.

@lucasw
Copy link
Author

lucasw commented May 6, 2022

The practice of declaring the Bind placeholders (_1, _2, ...) in the global namespace is deprecated.

Then use the qualified names like boost::placeholders::_1 directly, or locally do using namespace boost::placeholders

There's mention of including <boost/bind/bind.hpp> instead of <boost/bind.hpp> but I didn't see any existing bind.hpp includes so didn't make any changes there. I'd have to look further but I think the old include that defines the macro for preserving the old global namespace _1/_2 is removed entirely in later versions of boost, and compiling against those is what is motivating this PR.

https://stackoverflow.com/questions/53203970/why-boostbind-insists-pulling-boostplaceholders-into-global-namespace

Copy link
Collaborator

@ayrton04 ayrton04 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I think it's the right call to be explicit and use, e.g., boost::placeholders::_1 instead of _1. But with noetic being end-of-the-line for ROS 1, I'm not sure we'll gain much if we later try to move to the STL equivalents (though I obviously prefer them in general).

@@ -49,8 +49,6 @@ if(NOT EIGEN3_FOUND)
set(EIGEN_PACKAGE Eigen)
endif()

set(CMAKE_CXX_STANDARD 14)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this need to be removed?

Copy link
Author

@lucasw lucasw May 20, 2022

Choose a reason for hiding this comment

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

It's a log4cxx 0.12 issue that results in

/usr/include/log4cxx/boost-std-configuration.h:10:18: error: ‘shared_mutex’ in namespace ‘std’ does not name a type
   10 |     typedef std::shared_mutex shared_mutex;
      |                  ^~~~~~~~~~~~```

when building with < C++17.

Ubuntu 20.04 noetic already defaults to C++14 and has an older log4cxx, newer systems with newer log4cxx default to C++17, and no need to support older systems in this noetic branch, so removing it seems cleanest, but I can see setting it to 17 instead. Also putting it into a separate commit in this PR, or a separate PR (or don't accept if at all if you prefer, it's easy to maintain a fork with one line of difference but much nicer for others with the newer log4cxx not to have to).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with the branch assuming whatever standard is adopted by the ROS release in question. Thanks for the info!

@ayrton04 ayrton04 merged commit 3d94b08 into cra-ros-pkg:noetic-devel May 24, 2022
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

3 participants