[libc++] Simplify the string structures a bit more

[philnik]

Summary

This simplifies the string structs a bit more and the normal layout should not contain any undefined behaviour anymore. I don't think there is a way to achieve this in the alternate string mode without breaking the ABI.

[ldionne]

Other than my comment, LGTM.

libcxx/include/string

738–741

Here and above, we assume that 7+1 == 8 == CHAR_BIT, since the padding could potentially be wrong if that were not the case. I think this is a pre-existing condition, but I would suggest asserting it. Basically, I think our implementation doesn't support systems where CHAR_BIT != 8. WDYT?

[philnik]

• Add assertion

[hubert.reinterpretcast]

libcxx/include/string

740

Is it really okay if the expression evaluates to zero?

[philnik]

libcxx/include/string

740

Clang and GCC allow arrays with zero size. https://godbolt.org/z/ddPMThYMz

[aprantl]

Note that this broke the LLDB data formatters.

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/lastFailedBuild/

I put up a patch to adapt them in https://reviews.llvm.org/D126080.

[ayzhao]

Chrome is observing failures on Windows caused by this patch: https://crbug.com/1334441

[Mordante]

libcxx/include/string

695

When you fix this can you add an static_assert that validates the size of the struct?

[hubert.reinterpretcast]

In D125496#3575088, @ayzhao wrote:

Chrome is observing failures on Windows caused by this patch: https://crbug.com/1334441

I will note this patch fixes an ABI break on AIX caused by https://reviews.llvm.org/D123580.

[philnik]

In D125496#3575712, @hubert.reinterpretcast wrote:

In D125496#3575088, @ayzhao wrote:

Chrome is observing failures on Windows caused by this patch: https://crbug.com/1334441

I will note this patch fixes an ABI break on AIX caused by https://reviews.llvm.org/D123580.

Do you want to tell me that using size_type is correct for AIX and unsigned char is correct for Windows?

[hubert.reinterpretcast]

In D125496#3575714, @philnik wrote:

Do you want to tell me that using size_type is correct for AIX and unsigned char is correct for Windows?

No, I was pointing out that a plain revert would be problematic. If the proposal is to change the type of the bit-fields to unsigned char, then that should be compatible on AIX as well.

libcxx/include/string

static_assert(( is_same<typename allocator_type::value_type, value_type>::value),
                   "Allocator::value_type must be same type as value_type");
 
     typedef __wrap_iter<pointer>                         iterator;
     typedef __wrap_iter<const_pointer>                   const_iterator;
     typedef std::reverse_iterator<iterator>              reverse_iterator;
     typedef std::reverse_iterator<const_iterator>        const_reverse_iterator;
 
 private:
+    static_assert(CHAR_BIT == 8, "This implementation assumes that one byte contains 8 bits");
 
 #ifdef _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 
     struct __long
     {
         pointer   __data_;
         size_type __size_;
         size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
         size_type __is_long_ : 1;
     };
 
     enum {__min_cap = (sizeof(__long) - 1)/sizeof(value_type) > 2 ?
                       (sizeof(__long) - 1)/sizeof(value_type) : 2};
 
     struct __short
     {
         value_type __data_[__min_cap];
-        unsigned char __padding[sizeof(value_type) - 1];
-        unsigned char __size_ : 7;
-        unsigned char __is_long_ : 1;
+        unsigned char __padding_[sizeof(value_type) - 1];
+        size_type __size_ : 7;
+        size_type __is_long_ : 1;
     };
 
 // The __endian_factor is required because the field we use to store the size
-// (either size_type or unsigned char depending on long/short) has one fewer
-// bit than it would if it were not a bitfield.
+// has one fewer bit than it would if it were not a bitfield.
 //
 // If the LSB is used to store the short-flag in the short string representation,
 // we have to multiply the size by two when it is stored and divide it by two when
 // it is loaded to make sure that we always store an even number. In the long string
 // representation, we can ignore this because we can assume that we always allocate
 // an even amount of value_types.
 //
 // If the MSB is used for the short-flag, the max_size() is numeric_limits<size_type>::max() / 2.
struct __long
         pointer   __data_;
     };
 
     enum {__min_cap = (sizeof(__long) - 1)/sizeof(value_type) > 2 ?
                       (sizeof(__long) - 1)/sizeof(value_type) : 2};
 
     struct __short
     {
-        union
-        {
-            struct {
-                unsigned char __is_long_ : 1;
-                unsigned char __size_ : 7;
-            };
-            value_type __lx;
-        };
+        size_type __is_long_ : 1;
+        size_type __size_ : 7;
+        char __padding_[sizeof(value_type) - 1];
         value_type __data_[__min_cap];
     };
 
 #endif // _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 
     union __ulx{__long __lx; __short __lxx;};
 
     enum {__n_words = sizeof(__ulx) / sizeof(size_type)};