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

Feature: support any allocator in offset_ptr_stl_allocator #63

Merged
merged 13 commits into from
Dec 11, 2024

Conversation

liss-h
Copy link
Contributor

@liss-h liss-h commented Dec 6, 2024

Support any stl-like allocator in offset_ptr_stl_allocator not just std::allocator

Copy link
Member

@bigerl bigerl left a comment

Choose a reason for hiding this comment

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

Thx for PR. Looks good already, I would mainly sharpen the interface of offset_ptr_stl_allocator. With the requested changes, we could rename it to offset_ptr_allocator or offset_ptr_allocator_wrapper.

constexpr offset_ptr_stl_allocator &operator=(offset_ptr_stl_allocator &&other) noexcept(std::is_nothrow_move_assignable_v<upstream_allocator_type>) = default;

explicit constexpr offset_ptr_stl_allocator(upstream_allocator_type const &upstream) noexcept(std::is_nothrow_copy_constructible_v<upstream_allocator_type>)
: inner_{upstream} {
Copy link
Member

Choose a reason for hiding this comment

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

only activate if upstream_allocator_type is copy constructible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there allocators that are not copy constructible? Would that work? I think all containers assume that they are copy constructible.

The only thing I'm aware of is allocators that are not assignable or swappable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • There are only constants to customize assignment and swap behaviour
  • There is also select_on_container_copy_construction which is required to create a new instance.
  • all container ctors take allocator by const &

Copy link
Member

Choose a reason for hiding this comment

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

about copy construction: you are right "(Note: Every Allocator also satisfies CopyConstructible.)" from https://en.cppreference.com/w/cpp/named_req/Allocator

Copy link
Member

Choose a reason for hiding this comment

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

reviewing the link above I come to the conclusion that every allocator must have move/copy assignment and copy ctor. I couldn't find anything about move ctor but I think it is safe to assume that it will always be present if there is already a move assignment operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are wrong about assignment, there is nothing about assignment being required on that page you sent me. Only move+copy ctors.

copy ctor
grafik

move ctor
grafik

This can also be confirmed by looking at std::pmr::polymorphic_allocator as it does not have any operator=.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I've now defaulted the copy and move ctor, as it does not make sense to implement them manually

include/dice/template-library/polymorphic_allocator.hpp Outdated Show resolved Hide resolved
@liss-h liss-h merged commit 631a536 into develop Dec 11, 2024
6 checks passed
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.

2 participants