This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove __deque_base
ClosedPublic

Authored by philnik on Aug 17 2022, 4:24 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG59d246e55f56: [libc++] Remove __deque_base
Summary

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::.

Diff Detail

Event Timeline

philnik created this revision.Aug 17 2022, 4:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 4:24 PM
philnik requested review of this revision.Aug 17 2022, 4:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 4:24 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 453602.Aug 18 2022, 3:04 AM
  • Add C++03 spaces
philnik updated this revision to Diff 453611.Aug 18 2022, 3:38 AM
  • Use TEST_ALIGNOF
philnik updated this revision to Diff 453625.Aug 18 2022, 5:14 AM
  • Try to fix CI
ldionne requested changes to this revision.Aug 18 2022, 9:20 AM

Looks mostly good, but I'd like to see it again before it goes in.

libcxx/include/deque
1037

We need to spam _LIBCPP_HIDE_FROM_ABI here and in other places. Fine to do in a follow-up.

1057–1059

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.

This revision now requires changes to proceed.Aug 18 2022, 9:20 AM
philnik updated this revision to Diff 454260.Aug 20 2022, 4:55 PM
philnik marked an inline comment as done.
  • Address comments
philnik marked an inline comment as done.
philnik added inline comments.
libcxx/include/deque
1037

See D132320.

philnik updated this revision to Diff 454529.Aug 22 2022, 8:58 AM
philnik marked an inline comment as done.
  • Try to get the right ABI numbers
ldionne accepted this revision.Aug 25 2022, 9:26 AM

LGTM with green CI and my comments applied.

libcxx/include/deque
1045–1048

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.

This revision is now accepted and ready to land.Aug 25 2022, 9:26 AM
philnik updated this revision to Diff 455700.Aug 25 2022, 1:15 PM
philnik marked an inline comment as done.
  • Address comments
philnik updated this revision to Diff 455706.Aug 25 2022, 1:47 PM
  • Rebased
philnik updated this revision to Diff 455932.Aug 26 2022, 8:51 AM
  • Next try
This revision was automatically updated to reflect the committed changes.

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?

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.

Thanks, @philnik ; that patch indeed works. I appreciate the quick reply, too.