-
Notifications
You must be signed in to change notification settings - Fork 873
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
Use default CXX, use boost::placeholders::_1 instead of _1 #750
Conversation
Is there any benefit to this? Mixing and matching doesn't seem like a great policy to maintain |
eb4493c
to
9a95c78
Compare
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. |
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. |
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 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) |
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.
Why did this need to be removed?
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.
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).
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'm fine with the branch assuming whatever standard is adopted by the ROS release in question. Thanks for the info!
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.