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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Given that we end up with a lot of conditionals when removing this, I don't think this is worth doing. We don't gain anything performance wise, and we actually lose something clarity wise because this makes the code more complicated. If we had been able to move the members out of `__vector_base` without breaking the ABI, I think this would have been compelling, but now I'm not sure. WDYT?



================
Comment at: libcxx/include/vector:311
     // Both are defined in vector.cpp
     _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_length_error() const;
     _LIBCPP_NORETURN _LIBCPP_EXPORTED_FROM_ABI void __throw_out_of_range() const;
----------------
`vector.cpp` should not export the functions if `_LIBCPP_ABI_NO_VECTOR_BASE_CLASS` is not defined.


================
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
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117108