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

Switch to std::allocator_traits. #564

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

clalancette
Copy link
Contributor

In C++17, Allocator::rebind<>::other is deprecated (and Windows
now warns about it). See
https://en.cppreference.com/w/cpp/memory/allocator for more information.
Switch to std::allocator_traits which works in both C++14 and C++17.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

In C++17, Allocator::rebind<>::other is deprecated (and Windows
now warns about it).  See
https://en.cppreference.com/w/cpp/memory/allocator for more information.
Switch to std::allocator_traits which works in both C++14 and C++17.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

CI:

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

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

On a related, but not blocking note, Dirk and I realized sometime last year that using custom allocators with our rclcpp API just doesn't work at all. It may still be useful to use this allocator with the message without using it with rclcpp, but that seems like a small likelyhood. One of the possible solutions was to switch to the polymorphic allocator. Having C++17 now opens that possibility up to us as it was added in C++17 and modified somewhat heavily in C++20:

https://en.cppreference.com/w/cpp/memory/polymorphic_allocator

But I still think it would be the right thing to use even now without C++20.

@clalancette
Copy link
Contributor Author

One of the possible solutions was to switch to the polymorphic allocator. Having C++17 now opens that possibility up to us as it was added in C++17 and modified somewhat heavily in C++20:

https://en.cppreference.com/w/cpp/memory/polymorphic_allocator

But I still think it would be the right thing to use even now without C++20.

Makes sense. Is there an issue tracking that problem? If not, would you mind opening one (just because I don't have the context here).

The failing tests in CI are failing in the nightlies as well, so those are nothing new. I'm going to go ahead and merge this, thanks for the review.

@clalancette clalancette merged commit 681e833 into master Jan 26, 2021
@clalancette clalancette deleted the clalancette/switch-to-allocator-traits branch January 26, 2021 21:09
@wjwwood
Copy link
Member

wjwwood commented Jan 26, 2021

Makes sense. Is there an issue tracking that problem? If not, would you mind opening one (just because I don't have the context here).

Good question, I'll look.

@wjwwood
Copy link
Member

wjwwood commented Jan 26, 2021

I've opened an issue here: #566

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros2-compatibility-with-c-20/19014/3

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