[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 inline comments.


================
Comment at: libcxx/include/string:681
+        size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
+        bool __is_long_ : 1;
     };
----------------
philnik wrote:
> 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?
Good to know.
BTW it's not AIX but win32 where the different types are an issue. (AIX had something else with bitfields)
https://godbolt.org/z/n3zr5v4sr The different types make the struct 32 bytes on Windows.

I don't know where the information regarding the bit field layout can be found.
(I guess Google or the LLVM sources can help out.)


================
Comment at: libcxx/include/string:703
+
+#ifdef _LIBCPP_BIG_ENDIAN
+    static const size_type __endian_factor = 1;
----------------
philnik wrote:
> 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.
I see, I missed the `#else` on line 701.


================
Comment at: libcxx/include/string:1486
+        if (__s > 127)
+            __libcpp_unreachable();
+        __r_.first().__s.__size_ = __s;
----------------
philnik wrote:
> 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. 
>From a maintainability PoV I like `_LIBCPP_ASSERT` better. Would it be an option to adjust `_LIBCPP_ASSERT?` the "generate" this code when using GCC?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123580