[libcxx-commits] [PATCH] D123580: [libc++] Use bit field for checking if string is in long or short mode

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general I like this direction, but I'm somewhat concerned regarding ABI breaks.



================
Comment at: libcxx/include/string:681
+        size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
+        bool __is_long_ : 1;
     };
----------------
Do we have a test that validates the size of this struct?

On some platforms (IIRC AIX) the members `__cap_` and `__is_long_` won't be stored in one `size_type` since they have different types.


================
Comment at: libcxx/include/string:681
+        size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
+        bool __is_long_ : 1;
     };
----------------
Mordante wrote:
> Do we have a test that validates the size of this struct?
> 
> On some platforms (IIRC AIX) the members `__cap_` and `__is_long_` won't be stored in one `size_type` since they have different types.
Did you verify this isn't an ABI break when libc++14 and libc++15 compiled code is mixed?
We should make sure that doesn't break an big and little endian.


================
Comment at: libcxx/include/string:696
+#ifdef _LIBCPP_BIG_ENDIAN
+    static const size_type __endian_factor = 2;
 #else
----------------
Since you now understand how this works, it would be nice to add some documentation.


================
Comment at: libcxx/include/string:703
+
+#ifdef _LIBCPP_BIG_ENDIAN
+    static const size_type __endian_factor = 1;
----------------
Is this a copy paste?


================
Comment at: libcxx/include/string:1486
+        if (__s > 127)
+            __libcpp_unreachable();
+        __r_.first().__s.__size_ = __s;
----------------
Mainly curious, but why not a `_LIBCPP_ASSERT`?


================
Comment at: libcxx/utils/gdb/libcxx/printers.py:192
 
 class StdStringPrinter(object):
     """Print a std::string."""
----------------
Does this also break the LLDB pretty printer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123580