[libcxx-commits] [libcxx] [libc++][string] Fix unnecessary padding for basic_string's __short (PR #135973)

[via libcxx-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: Gábor Spaits (spaits)

<details>
<summary>Changes</summary>

Originally, padding in the struct `__short` was done with arrays, but when no padding was required, we got undefined behavior due to a zero-length array. C++17 11.3.4 Arrays p1:
```
If the <<length of array>> is present, it shall be a converted constant
expression of type std::size_t and its value shall be greater than zero.
```
A fix was proposed in commit d95597dc06c510ad7fbf00a43583c54d38f79aa7, which worked around the undefined behavior by wrapping the padding array in a struct if there was padding required and omitting the array altogether, with the help of a specialization, when the padding is zero.

This change doesn't just work around the problem but changes the semantics of the code, introducing an issue.

Contrary to C, in C++ empty structs and classes must have a size greater than zero. C++ 17 12 Classes p4:
```
Complete objects and member subobjects of class type shall have nonzero size.
```
So whenever there is no padding required we insert an empty struct-sized padding, which is 1 byte on X86_64 (but we won't feel it there, since on that platform padding is required if I am correct).

A standard compliant solution, that keeps to the original behavior could be done using unnamed 0 width bit-fields. 

12.2.4 Bit-fields p2:
```
As a special case, an unnamed bit-field with a width of zero
specifies alignment of the next bit-field at an allocation unit boundary. Only when declaring an unnamed
bit-field may the value of the constant-expression be equal to zero.
```

---
Full diff: https://github.com/llvm/llvm-project/pull/135973.diff


1 Files Affected:

- (modified) libcxx/include/string (+2-10) 


``````````diff
diff --git a/libcxx/include/string b/libcxx/include/string
index e7e541e31432d..c82cb5074e5d9 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -715,14 +715,6 @@ struct __can_be_converted_to_string_view
 struct __uninitialized_size_tag {};
 struct __init_with_sentinel_tag {};
 
-template <size_t _PaddingSize>
-struct __padding {
-  char __padding_[_PaddingSize];
-};
-
-template <>
-struct __padding<0> {};
-
 template <class _CharT, class _Traits, class _Allocator>
 class basic_string {
 public:
@@ -827,7 +819,7 @@ private:
 
   struct __short {
     value_type __data_[__min_cap];
-    _LIBCPP_NO_UNIQUE_ADDRESS __padding<sizeof(value_type) - 1> __padding_;
+    value_type : 0;
     unsigned char __size_    : 7;
     unsigned char __is_long_ : 1;
   };
@@ -879,7 +871,7 @@ private:
       unsigned char __is_long_ : 1;
       unsigned char __size_    : 7;
     };
-    _LIBCPP_NO_UNIQUE_ADDRESS __padding<sizeof(value_type) - 1> __padding_;
+    value_type : 0;
     value_type __data_[__min_cap];
   };
 

``````````

</details>


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