[libcxx-commits] [PATCH] D117108: [libc++] Remove vector base class

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/vector:316
 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
----------------
ldionne wrote:
> I was trying to convince myself that it wouldn't be ABI breaking to do the following:
> 
> ```
> template <bool> struct __vector_base_common { ... };
> template <class, class> struct __vector_base : private __vector_base_common<true> { /* nothing here */ };
> 
> template <class _Tp, class _Allocator>
> class vector
> #if !defined(_LIBCPP_ABI_NO_VECTOR_BASE_CLASS)
>   : private __vector_base<_Tp, _Allocator>
> #endif
> {
>   pointer                                                      __begin_;
>   pointer                                                      __end_;
>   __compressed_pair<pointer, allocator_type>                   __end_cap_;
> 
>   // use members normally here, unconditionally
> };
> ```
> 
> However, the usual pitfall of multiply inheritting from `vector<T>` applies here too. Let's assume we have:
> 
> ```
> struct IntVector1 : std::vector<int> { };
> struct IntVector2 : std::vector<int> { };
> struct Foo : IntVector1, IntVector2 { };
> ```
> 
> Before the change, the layout should be:
> 
> ```
> +---------------------------------+
> | __vector_base_common<true>      | 0 bytes, EBO'd
> | __vector_base<int>::__begin_    | 8 bytes
> | __vector_base<int>::__end_      | 8 bytes
> | __vector_base<int>::__end_cap_  | 8 bytes (disregarding allocator)
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
> | __vector_base_common<true>      | 1 byte, can't EBO
> | __vector_base<int>::__begin_    | 8 bytes
> | __vector_base<int>::__end_      | 8 bytes
> | __vector_base<int>::__end_cap_  | 8 bytes (disregarding allocator)
> +---------------------------------+
> ```
> 
> After the change, the layout should be:
> 
> ```
> +---------------------------------+
> | __vector_base_common<true>      | 0 bytes, EBO'd
> | __vector_base<int>              | 0 bytes, EBO'd
> | vector<int>::__begin_           | 8 bytes
> | vector<int>::__end_             | 8 bytes
> | vector<int>::__end_cap_         | 8 bytes (disregarding allocator)
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
> | __vector_base_common<true>      | 1 byte, can't EBO
> | __vector_base<int>              | 1 byte, can't EBO <<-- this is the ABI break
> | vector<int>::__begin_           | 8 bytes
> | vector<int>::__end_             | 8 bytes
> | vector<int>::__end_cap_         | 8 bytes (disregarding allocator)
> +---------------------------------+
> ```
> 
> Since `__vector_base<int>` would now be empty, it would get EBO'd the first time it's a base class, but not the second time, so there would be one extra byte because of that "spurious" base class.
> 
> Long story short, my suggestion doesn't work because it would break multiple inheritance of `std::vector`.
https://godbolt.org/z/rdMfbchKq seems to suggest that something with your tables is wrong. My current diff has still the same size with your example.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117108/new/

https://reviews.llvm.org/D117108