[libcxx-commits] [libcxx] [libcxx] adds a size-based representation for `vector`'s unstable ABI (PR #155330)

[Christopher Di Bella via libcxx-commits]

================
@@ -82,25 +83,334 @@ _LIBCPP_PUSH_MACROS
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#ifdef _LIBCPP_ABI_SIZE_BASED_VECTOR
----------------
cjdb wrote:

> Can you say why you created a separate `__vector_layout` type, vs ifdefs in the body of `class vector`? It feels like this factoring triggers the need for a LOT more modifications in this file than would otherwise be necessary. And I'm not sure what actual advantage is seen from having it.
>
> For example, none of the diffs relating to `__alloc_` and `__begin_` would be here, if not for the addition of the `__vector_layout` type.
>
> I think it certainly makes sense to group the ifdef-conditionalized code together by introducing internal helper functions -- as you have done. But that same effect can still be had without also introducing a new base class type.

Good question! The goal of `__vector_layout` was derived from `__split_buffer_(pointer|size)_layout`, which needs to be its own class because `vector` and `deque` require different layouts (there's currently no evidence that changing `deque`'s ABI to use `__split_buffer_pointer_layout` is beneficial, and it caused problems somewhere). We decided that it would be best from a software engineering standpoint to have a little redundancy in having a separate class, to keep `__split_buffer`'s logic as easily readable as possible (the trade-off was a more difficult PR diff, but substantially less context switching in the logic, and complete encapsulation). The changes that I made to `vector` rhyme very heavily with the changes made in #139632, so I figured it would be best to keep the design as similar as possible.

That said, I'm interested to explore your suggestion for two reasons:

1. You think it might reduce the diff, and if you're right, it'll probably help speed up reviewing this patch.
2. It might help with the debugger pretty-printers, something I'm still working on.

> On a trivial note, I'm not sure what the usual policy in libc++ is on ifdef ordering, but I was initially confused that the "old" layout was not textually the first ifdef'd half.

Interestingly, you're the first person to have brought this up (I expected it to have been flagged in the first patch). My rationale is that libc++ might want to support other layouts, and that ifdeffing is easier when done in a positive form, than a negative form.

https://github.com/llvm/llvm-project/pull/155330