This patch simplifies the implementation of deque by removing the __deque_base class which results in a lot less indirections and removes the need for __base::.
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG59d246e55f56: [libc++] Remove __deque_base
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks mostly good, but I'd like to see it again before it goes in.
libcxx/include/deque | ||
---|---|---|
1046 | We need to spam _LIBCPP_HIDE_FROM_ABI here and in other places. Fine to do in a follow-up. | |
1066–1068 | I would not use default member initalizers because that is a potential change in semantics. I'd be fine with the change if it were done in its separate patch where we can focus on the potential effects of it. In particular, undoing this should force you to really "copy" the definition of the various __deque_base constructors, which I can see have changed in the current version of the patch. |
LGTM with green CI and my comments applied.
libcxx/include/deque | ||
---|---|---|
1054–1057 | I had never noticed this, but we have two allocators. One is in the __compressed_pair we see right here, and the other one is in the __map, which is a __split_buffer. This is weird. I think this means that we can actually be lying when we set the noexcept-ness of deque() and friends. I think we should add this: static_assert(is_nothrow_default_constructible<allocator_type>::value == is_nothrow_default_constructible<_SplitBufferAllocator>::value, ""); static_assert(is_nothrow_move_constructible<allocator_type>::value == is_nothrow_move_constructible<_SplitBufferAllocator>::value, ""); // etc... I can't imagine this not being true, however this would provide guard rails and it would nicely document the code. |
I'm seeing new compilation errors from the moved allocator static_assert in this patch.
Reduced test case:
#include <deque> #include <memory> BaseContainerTtemplate <typename T> struct CustomAllocator : public std::allocator<T> {}; void foo() {std::deque<int, CustomAllocator<int>> bar;}
Error:
include/c++/v1/deque:974:3: error: static assertion failed due to requirement 'is_same<CustomAllocator<int>, std::allocator<int>>::value': rebinding an allocator to value_type should result in the original allocator static_assert(is_same<allocator_type, typename __rebind_alloc_helper<__alloc_traits, value_type>::type>::value, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ test.cc:4:51: note: in instantiation of template class 'std::deque<int, CustomAllocator<int>>' requested here void foo() {std::deque<int, CustomAllocator<int>> bar;} ^
Could you please investigate?
The static_assert wasn't moved, it was added. As the error message says, rebinding an allocator to the same type has to result in the same allocator. You have to add something like
template <class U> struct rebind { using other = allocator<U>; };
to your allocator pre-C++20.
We need to spam _LIBCPP_HIDE_FROM_ABI here and in other places. Fine to do in a follow-up.