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

Remove std::binary_function usage #561

Merged
merged 1 commit into from
Oct 2, 2018
Merged

Remove std::binary_function usage #561

merged 1 commit into from
Oct 2, 2018

Conversation

aladram
Copy link
Contributor

@aladram aladram commented Sep 25, 2018

This pull request removes an inherit of std::binary_function for the struct strcmp_wrapper, a C-style string comparator.

The struct strcmp_wrapper is used by the IDTopicMap typedef (line 276) which is a std::map, and std::map requires strcmp_wrapper to fulfill the Compare named requirement. The inherit of std::binary_function only defines 3 types: first_argument_type, second_argument_type and result_type (typedefs of types passed in argument), which is useless here because they are not part of the requirement.

std::binary_function has been deprecated in C++11, removed in C++17. This change may seem useless, but this change enables us to compile C++17 ROS2 packages using rclcpp (or at least some of them), which may be useful.

Deprecated in C++11, removed in C++17
@tfoote tfoote added the in review Waiting for review (Kanban column) label Sep 25, 2018
@wjwwood
Copy link
Member

wjwwood commented Sep 25, 2018

I wrote this originally, but I don't remember much about it :D, and I was probably following some online recommendation on how to make the strcmp wrapper.

This seems reasonable to me, but we'll have to run CI on it to see if it breaks older compilers/c++ standards or not.

@wjwwood
Copy link
Member

wjwwood commented Sep 25, 2018

Here's CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@aladram
Copy link
Contributor Author

aladram commented Sep 25, 2018

Running CI on a pull request, this also seems reasonable to me! ;) Thanks for triggering the CI build.

@aladram
Copy link
Contributor Author

aladram commented Sep 27, 2018

Any news? CI looks fine (a test fails for Linux build, but it started to fail on the previous build, and anyway my change does not affect runtime but compile time).

@wjwwood
Copy link
Member

wjwwood commented Oct 2, 2018

lgtm to me, I agree the test failure is unrelated.

@wjwwood wjwwood merged commit eb439dd into ros2:master Oct 2, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Oct 2, 2018
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