Skip to content

Switch to std::allocator_traits.#564

Merged
clalancette merged 1 commit intomasterfrom
clalancette/switch-to-allocator-traits
Jan 26, 2021
Merged

Switch to std::allocator_traits.#564
clalancette merged 1 commit intomasterfrom
clalancette/switch-to-allocator-traits

Conversation

@clalancette
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

CI:

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

wjwwood commented Jan 26, 2021

I've opened an issue here: #566

@ros-discourse
Copy link
Copy Markdown

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.

3 participants