[libc++][NFC] Inline most of `__vector_base` into `vector`.

[var-const]

Summary

__vector_base exists for historical reasons and cannot be eliminated
entirely without breaking the ABI. Member variables are left
untouched -- this patch only does changes that clearly cannot affect the
ABI.

[var-const]

libcxx/include/vector

322–324

My understanding is that it's probably okay to move the member variables as well (from the ABI perspective), but I'd rather do that change in a follow-up.

436–439

It seemed that newer code in vector tends to use 2 spaces for indentation, so I reindented the older code I was moving around to be consistent with that.

[philnik]

libcxx/include/vector

813

Since you are moving this around, could you reformat to the LLVM style instead of keeping the old style?

[ldionne]

libcxx/include/vector

322–324

Yes, I think it's OK to move them as long as we keep the order and we keep the same base classes.

[var-const]

libcxx/include/vector

813

I'll see if I can get clang-format to work on this file. If that goes well, I'd rather do a separate patch for reformatting the whole file.

[var-const]

Rebase on main.

[Mordante]

libcxx/include/vector

813

I think we should be wary about formatting the entire file. It may cause merge conflicts for patches under review. I'd rather see a separate patch to format only the new lines new part in this file.

When you do that can you directly do s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/ for that part?

[• Quuxplusone]

LGTM mod kinda-pre-existing comments.

libcxx/include/vector

845–847

FWIW, this manual noexcept-specification scares me. Nobody actually checks whether this function is noexcept, do they? If so, then all we're doing via this specification is asking the compiler to insert a call to std::terminate in the more-or-less-likely case that we got the condition wrong. I'd rather just remove lines 845–847.
(Either this removal will make an existing regression test fail; or else it's totally a good idea; or else we need to add a regression test for it. I believe #2 is the right hypothesis.)

885–895

I'd prefer to see these tag-dispatch helpers moved up directly under the single-argument __move_assign_alloc that uses them. Ditto for __copy_assign_alloc. Bonus points if we can find a better function name for the helper, e.g. __move_assign_alloc_impl, so that we don't have one- and two-argument overloads of the same name.

[var-const]

Rebase on main.

[shafik]

Hello,

Just a reminder when we change underlying structure of types in libc++ it is important to run the LLDB test suite. We have many formatters for libc++ and the rely on a lot of internals for the formatting. This case did not break a formatter it did change the type being returned by several member functions.

I had to track down this break using bisect and I just applied a fix: 0d62e31c458526e1db37d11da947e61768c7b522

Thank you

[var-const]

Just a reminder when we change underlying structure of types in libc++ it is important to run the LLDB test suite. We have many formatters for libc++ and the rely on a lot of internals for the formatting. This case did not break a formatter it did change the type being returned by several member functions.

@shafik Thanks for the fix, and sorry about the trouble. I plan some related follow-ups and will keep the LLDB tests in mind in the future. Thanks!

[ldionne]

In D112976#3122551, @shafik wrote:

Hello,

Just a reminder when we change underlying structure of types in libc++ it is important to run the LLDB test suite. We have many formatters for libc++ and the rely on a lot of internals for the formatting. This case did not break a formatter it did change the type being returned by several member functions.

I had to track down this break using bisect and I just applied a fix: 0d62e31c458526e1db37d11da947e61768c7b522

We have to find a solution for this. Depending on what you mean by the "structure" of things, I suspect we are changing that all the time.

How long does it take to run the LLDB formatters? Could we add a CI job that does exactly that? If we don't implement a systematic way of catching these bugs, we will keep breaking you without intending to. Can you provide a minimal CMake + Lit invocation to run the LLDB formatters (and ideally just that to save time)? I'll look into adding a CI job for it.

libcxx/include/vector

struct __vector_base_common<true> {
     _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const;
     _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
 };
 
 template <class _Tp, class _Allocator>
 class __vector_base
     : protected __vector_base_common<true> // This base class is historical, but it needs to remain for ABI compatibility
 {
-public:
-    typedef _Allocator                               allocator_type;
-    typedef allocator_traits<allocator_type>         __alloc_traits;
-    typedef typename __alloc_traits::size_type       size_type;
-protected:
-    typedef _Tp                                      value_type;
-    typedef value_type&                              reference;
-    typedef const value_type&                        const_reference;
-    typedef typename __alloc_traits::difference_type difference_type;
-    typedef typename __alloc_traits::pointer         pointer;
-    typedef typename __alloc_traits::const_pointer   const_pointer;
-    typedef pointer                                  iterator;
-    typedef const_pointer                            const_iterator;
+    typedef _Allocator                                           allocator_type;
+    typedef typename allocator_traits<allocator_type>::pointer   pointer;
 
-    pointer                                         __begin_;
-    pointer                                         __end_;
-    __compressed_pair<pointer, allocator_type> __end_cap_;
+protected:
+    pointer                                                      __begin_;
+    pointer                                                      __end_;
+    __compressed_pair<pointer, allocator_type>                   __end_cap_;
 
     _LIBCPP_INLINE_VISIBILITY
-    allocator_type& __alloc() _NOEXCEPT
-        {return __end_cap_.second();}
-    _LIBCPP_INLINE_VISIBILITY
-    const allocator_type& __alloc() const _NOEXCEPT
-        {return __end_cap_.second();}
-    _LIBCPP_INLINE_VISIBILITY
-    pointer& __end_cap() _NOEXCEPT
-        {return __end_cap_.first();}
-    _LIBCPP_INLINE_VISIBILITY
-    const pointer& __end_cap() const _NOEXCEPT
-        {return __end_cap_.first();}
-
-    _LIBCPP_INLINE_VISIBILITY
     __vector_base()
-        _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value);
-    _LIBCPP_INLINE_VISIBILITY __vector_base(const allocator_type& __a);
-#ifndef _LIBCPP_CXX03_LANG
-    _LIBCPP_INLINE_VISIBILITY __vector_base(allocator_type&& __a) _NOEXCEPT;
-#endif
-    ~__vector_base();
-
-    _LIBCPP_INLINE_VISIBILITY
-    void clear() _NOEXCEPT {__destruct_at_end(__begin_);}
-    _LIBCPP_INLINE_VISIBILITY
-    size_type capacity() const _NOEXCEPT
-        {return static_cast<size_type>(__end_cap() - __begin_);}
-
-    _LIBCPP_INLINE_VISIBILITY
-    void __destruct_at_end(pointer __new_last) _NOEXCEPT;
-
-    _LIBCPP_INLINE_VISIBILITY
-    void __copy_assign_alloc(const __vector_base& __c)
-        {__copy_assign_alloc(__c, integral_constant<bool,
-                      __alloc_traits::propagate_on_container_copy_assignment::value>());}
-
-    _LIBCPP_INLINE_VISIBILITY
-    void __move_assign_alloc(__vector_base& __c)
-        _NOEXCEPT_(
-            !__alloc_traits::propagate_on_container_move_assignment::value ||
-            is_nothrow_move_assignable<allocator_type>::value)
-        {__move_assign_alloc(__c, integral_constant<bool,
-                      __alloc_traits::propagate_on_container_move_assignment::value>());}
-
-    _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
-    void __throw_length_error() const {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        __vector_base_common<true>::__throw_length_error();
-#else
-        _VSTD::abort();
-#endif
-    }
-
-    _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
-    void __throw_out_of_range() const {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        __vector_base_common<true>::__throw_out_of_range();
-#else
-        _VSTD::abort();
-#endif
-    }
-
-private:
-    _LIBCPP_INLINE_VISIBILITY
-    void __copy_assign_alloc(const __vector_base& __c, true_type)
-        {
-            if (__alloc() != __c.__alloc())
-            {
-                clear();
-                __alloc_traits::deallocate(__alloc(), __begin_, capacity());
-                __begin_ = __end_ = __end_cap() = nullptr;
-            }
-            __alloc() = __c.__alloc();
-        }
-
-    _LIBCPP_INLINE_VISIBILITY
-    void __copy_assign_alloc(const __vector_base&, false_type)
-        {}
-
-    _LIBCPP_INLINE_VISIBILITY
-    void __move_assign_alloc(__vector_base& __c, true_type)
-        _NOEXCEPT_(is_nothrow_move_assignable<allocator_type>::value)
-        {
-            __alloc() = _VSTD::move(__c.__alloc());
-        }
-
-    _LIBCPP_INLINE_VISIBILITY
-    void __move_assign_alloc(__vector_base&, false_type)
-        _NOEXCEPT
-        {}
-};
-
-template <class _Tp, class _Allocator>
-inline _LIBCPP_INLINE_VISIBILITY
-void
-__vector_base<_Tp, _Allocator>::__destruct_at_end(pointer __new_last) _NOEXCEPT
-{
-    pointer __soon_to_be_end = __end_;
-    while (__new_last != __soon_to_be_end)
-        __alloc_traits::destroy(__alloc(), _VSTD::__to_address(--__soon_to_be_end));
-    __end_ = __new_last;
-}
-
-template <class _Tp, class _Allocator>
-inline _LIBCPP_INLINE_VISIBILITY
-__vector_base<_Tp, _Allocator>::__vector_base()
         _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
       : __begin_(nullptr),
         __end_(nullptr),
-      __end_cap_(nullptr, __default_init_tag())
-{
-}
+        __end_cap_(nullptr, __default_init_tag()) {}
 
-template <class _Tp, class _Allocator>
-inline _LIBCPP_INLINE_VISIBILITY
-__vector_base<_Tp, _Allocator>::__vector_base(const allocator_type& __a)
+    _LIBCPP_INLINE_VISIBILITY __vector_base(const allocator_type& __a)
       : __begin_(nullptr),
         __end_(nullptr),
-      __end_cap_(nullptr, __a)
-{
-}
+        __end_cap_(nullptr, __a) {}
 
 #ifndef _LIBCPP_CXX03_LANG
-template <class _Tp, class _Allocator>
-inline _LIBCPP_INLINE_VISIBILITY
-__vector_base<_Tp, _Allocator>::__vector_base(allocator_type&& __a) _NOEXCEPT
+    _LIBCPP_INLINE_VISIBILITY __vector_base(allocator_type&& __a) _NOEXCEPT
       : __begin_(nullptr),
         __end_(nullptr),
         __end_cap_(nullptr, _VSTD::move(__a)) {}
 #endif
-
-template <class _Tp, class _Allocator>
-__vector_base<_Tp, _Allocator>::~__vector_base()
-{
-    if (__begin_ != nullptr)
-    {
-        clear();
-        __alloc_traits::deallocate(__alloc(), __begin_, capacity());
-    }
-}
+};
 
 template <class _Tp, class _Allocator /* = allocator<_Tp> */>
 class _LIBCPP_TEMPLATE_VIS vector
+    // This base class is historical, but it needs to remain for ABI compatibility.
     : private __vector_base<_Tp, _Allocator>
 {
 private:
     typedef __vector_base<_Tp, _Allocator>           __base;
     typedef allocator<_Tp>                           __default_allocator_type;
 public:
     typedef vector                                   __self;
     typedef _Tp                                      value_type;
     typedef _Allocator                               allocator_type;
-    typedef typename __base::__alloc_traits          __alloc_traits;
-    typedef typename __base::reference               reference;
-    typedef typename __base::const_reference         const_reference;
-    typedef typename __base::size_type               size_type;
-    typedef typename __base::difference_type         difference_type;
-    typedef typename __base::pointer                 pointer;
-    typedef typename __base::const_pointer           const_pointer;
+    typedef allocator_traits<allocator_type>         __alloc_traits;
+    typedef value_type&                              reference;
+    typedef const value_type&                        const_reference;
+    typedef typename __alloc_traits::size_type       size_type;
+    typedef typename __alloc_traits::difference_type difference_type;
+    typedef typename __alloc_traits::pointer         pointer;
+    typedef typename __alloc_traits::const_pointer   const_pointer;
     typedef __wrap_iter<pointer>                     iterator;
     typedef __wrap_iter<const_pointer>               const_iterator;
-    typedef _VSTD::reverse_iterator<iterator>         reverse_iterator;
-    typedef _VSTD::reverse_iterator<const_iterator>   const_reverse_iterator;
+    typedef _VSTD::reverse_iterator<iterator>        reverse_iterator;
+    typedef _VSTD::reverse_iterator<const_iterator>  const_reverse_iterator;
 
     static_assert((is_same<typename allocator_type::value_type, value_type>::value),
                   "Allocator::value_type must be same type as value_type");
 
     _LIBCPP_INLINE_VISIBILITY
     vector() _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
         {
 #if _LIBCPP_DEBUG_LEVEL == 2
template <class _ForwardIterator>
                typename enable_if<__is_cpp17_forward_iterator<_ForwardIterator>::value &&
                                  is_constructible<
                                     value_type,
                                     typename iterator_traits<_ForwardIterator>::reference>::value>::type* = 0);
 
     _LIBCPP_INLINE_VISIBILITY
     ~vector()
     {
-        __annotate_delete();
 #if _LIBCPP_DEBUG_LEVEL == 2
-        __get_db()->__erase_c(this);
 #endif
+
+      if (this->__begin_ != nullptr)
+      {
+        __clear();
+        __alloc_traits::deallocate(__alloc(), this->__begin_, capacity());
+      }
     }
 
     vector(const vector& __x);
     vector(const vector& __x, const __identity_t<allocator_type>& __a);
     _LIBCPP_INLINE_VISIBILITY
     vector& operator=(const vector& __x);
 
 #ifndef _LIBCPP_CXX03_LANG
#endif
     const_reverse_iterator crend()   const _NOEXCEPT
         {return rend();}
 
     _LIBCPP_INLINE_VISIBILITY
     size_type size() const _NOEXCEPT
         {return static_cast<size_type>(this->__end_ - this->__begin_);}
     _LIBCPP_INLINE_VISIBILITY
     size_type capacity() const _NOEXCEPT
-        {return __base::capacity();}
+        {return static_cast<size_type>(__end_cap() - this->__begin_);}
     _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY
     bool empty() const _NOEXCEPT
         {return this->__begin_ == this->__end_;}
     size_type max_size() const _NOEXCEPT;
     void reserve(size_type __n);
     void shrink_to_fit() _NOEXCEPT;
 
     _LIBCPP_INLINE_VISIBILITY reference       operator[](size_type __n) _NOEXCEPT;
#endif
 
     _LIBCPP_INLINE_VISIBILITY iterator erase(const_iterator __position);
     iterator erase(const_iterator __first, const_iterator __last);
 
     _LIBCPP_INLINE_VISIBILITY
     void clear() _NOEXCEPT
     {
         size_type __old_size = size();
-        __base::clear();
+        __clear();
         __annotate_shrink(__old_size);
         __invalidate_all_iterators();
     }
 
     void resize(size_type __sz);
     void resize(size_type __sz, const_reference __x);
 
     void swap(vector&)
void __move_assign(vector& __c, true_type)
         _NOEXCEPT_(is_nothrow_move_assignable<allocator_type>::value);
     void __move_assign(vector& __c, false_type)
         _NOEXCEPT_(__alloc_traits::is_always_equal::value);
     _LIBCPP_INLINE_VISIBILITY
     void __destruct_at_end(pointer __new_last) _NOEXCEPT
     {
         __invalidate_iterators_past(__new_last);
         size_type __old_size = size();
-        __base::__destruct_at_end(__new_last);
+        __base_destruct_at_end(__new_last);
         __annotate_shrink(__old_size);
     }
 
 #ifndef _LIBCPP_CXX03_LANG
     template <class _Up>
     _LIBCPP_INLINE_VISIBILITY
     inline void __push_back_slow_path(_Up&& __x);
 
#endif
   template <class ..._Args>
   _LIBCPP_INLINE_VISIBILITY
   void __construct_one_at_end(_Args&& ...__args) {
     _ConstructTransaction __tx(*this, 1);
     __alloc_traits::construct(this->__alloc(), _VSTD::__to_address(__tx.__pos_),
         _VSTD::forward<_Args>(__args)...);
     ++__tx.__pos_;
   }
+
+  _LIBCPP_INLINE_VISIBILITY
+  allocator_type& __alloc() _NOEXCEPT
+      {return this->__end_cap_.second();}
+  _LIBCPP_INLINE_VISIBILITY
+  const allocator_type& __alloc() const _NOEXCEPT
+      {return this->__end_cap_.second();}
+  _LIBCPP_INLINE_VISIBILITY
+  pointer& __end_cap() _NOEXCEPT
+      {return this->__end_cap_.first();}
+  _LIBCPP_INLINE_VISIBILITY
+  const pointer& __end_cap() const _NOEXCEPT
+      {return this->__end_cap_.first();}
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __clear() _NOEXCEPT {__base_destruct_at_end(this->__begin_);}
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __base_destruct_at_end(pointer __new_last) _NOEXCEPT {
+    pointer __soon_to_be_end = this->__end_;
+    while (__new_last != __soon_to_be_end)
+        __alloc_traits::destroy(__alloc(), _VSTD::__to_address(--__soon_to_be_end));
+    this->__end_ = __new_last;
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __copy_assign_alloc(const vector& __c)
+      {__copy_assign_alloc(__c, integral_constant<bool,
+                    __alloc_traits::propagate_on_container_copy_assignment::value>());}
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __move_assign_alloc(vector& __c)
+      _NOEXCEPT_(
+          !__alloc_traits::propagate_on_container_move_assignment::value ||
+          is_nothrow_move_assignable<allocator_type>::value)
+      {__move_assign_alloc(__c, integral_constant<bool,
+                    __alloc_traits::propagate_on_container_move_assignment::value>());}
+
+  _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
+  void __throw_length_error() const {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    __vector_base_common<true>::__throw_length_error();
+#else
+    _VSTD::abort();
+#endif
+  }
+
+  _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
+  void __throw_out_of_range() const {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    __vector_base_common<true>::__throw_out_of_range();
+#else
+    _VSTD::abort();
+#endif
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __copy_assign_alloc(const vector& __c, true_type)
+  {
+    if (__alloc() != __c.__alloc())
+    {
+      __clear();
+      __alloc_traits::deallocate(__alloc(), this->__begin_, capacity());
+      this->__begin_ = this->__end_ = __end_cap() = nullptr;
+    }
+    __alloc() = __c.__alloc();
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __copy_assign_alloc(const vector&, false_type)
+  {}
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __move_assign_alloc(vector& __c, true_type)
+      _NOEXCEPT_(is_nothrow_move_assignable<allocator_type>::value)
+  {
+    __alloc() = _VSTD::move(__c.__alloc());
+  }
+
+  _LIBCPP_INLINE_VISIBILITY
+  void __move_assign_alloc(vector&, false_type)
+      _NOEXCEPT
+  {}
 };
 
 #if _LIBCPP_STD_VER >= 17
 template<class _InputIterator,
          class _Alloc = allocator<__iter_value_type<_InputIterator>>,
          class = enable_if_t<__is_allocator<_Alloc>::value>
          >
 vector(_InputIterator, _InputIterator)
vector<_Tp, _Allocator>::operator=(vector&& __x)
     return *this;
 }
 
 template <class _Tp, class _Allocator>
 void
 vector<_Tp, _Allocator>::__move_assign(vector& __c, false_type)
     _NOEXCEPT_(__alloc_traits::is_always_equal::value)
 {
-    if (__base::__alloc() != __c.__alloc())
+    if (__alloc() != __c.__alloc())
     {
         typedef move_iterator<iterator> _Ip;
         assign(_Ip(__c.begin()), _Ip(__c.end()));
     }
     else
         __move_assign(__c, true_type());
 }
 
 template <class _Tp, class _Allocator>
 void
 vector<_Tp, _Allocator>::__move_assign(vector& __c, true_type)
     _NOEXCEPT_(is_nothrow_move_assignable<allocator_type>::value)
 {
     __vdeallocate();
-    __base::__move_assign_alloc(__c); // this can throw
+    __move_assign_alloc(__c); // this can throw
     this->__begin_ = __c.__begin_;
     this->__end_ = __c.__end_;
     this->__end_cap() = __c.__end_cap();
     __c.__begin_ = __c.__end_ = __c.__end_cap() = nullptr;
 #if _LIBCPP_DEBUG_LEVEL == 2
     __get_db()->swap(this, _VSTD::addressof(__c));
 #endif
 }
 
 #endif // !_LIBCPP_CXX03_LANG
 
 template <class _Tp, class _Allocator>
 inline _LIBCPP_INLINE_VISIBILITY
 vector<_Tp, _Allocator>&
 vector<_Tp, _Allocator>::operator=(const vector& __x)
 {
     if (this != _VSTD::addressof(__x))
     {
-        __base::__copy_assign_alloc(__x);
+        __copy_assign_alloc(__x);
         assign(__x.__begin_, __x.__end_);
     }
     return *this;
 }
 
 template <class _Tp, class _Allocator>
 template <class _InputIterator>
 typename enable_if