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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added inline comments.


================
Comment at: libcxx/include/string:681
+        size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
+        bool __is_long_ : 1;
     };
----------------
Mordante wrote:
> 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.
https://godbolt.org/z/axTMxqaj3 is the code I used to verify that everything is OK. I think this should be good enough to say that they are equivalent. Do you know if there is any documentation about the bit field layout on different platforms?


================
Comment at: libcxx/include/string:703
+
+#ifdef _LIBCPP_BIG_ENDIAN
+    static const size_type __endian_factor = 1;
----------------
Mordante wrote:
> Is this a copy paste?
No, depending on the string layout the factors have to be reversed. It's essentially: If you use the LSB for `is_long` you have to multiply by two, otherwise keep the same number.


================
Comment at: libcxx/include/string:1486
+        if (__s > 127)
+            __libcpp_unreachable();
+        __r_.first().__s.__size_ = __s;
----------------
Mordante wrote:
> Mainly curious, but why not a `_LIBCPP_ASSERT`?
This ensures optimal code generation. GCC doesn't have `__builtin_assume`. This is a workaround. 


================
Comment at: libcxx/utils/gdb/libcxx/printers.py:192
 
 class StdStringPrinter(object):
     """Print a std::string."""
----------------
Mordante wrote:
> Does this also break the LLDB pretty printer?
Probably. Would be nice to have a test runner for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123580