[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 accepted this revision.
Mordante added a subscriber: jingham.
Mordante added a comment.

LGTM, with some minor remarks. I think it would be good to wait for feedback from the LLDB devs to validate whether this breaks the data formatters.



================
Comment at: libcxx/include/string:707
+// This does not impact the short string representation, since we never need the MSB
+// for representing the size of a short string anyway.
+
----------------
Thanks for the description!


================
Comment at: libcxx/include/string:1499
+    void __set_short_size(size_type __s) _NOEXCEPT {
+        _LIBCPP_ASSERT(__s < __min_cap, "__s should never be greater or equal to the short string capacity");
+        __r_.first().__s.__size_ = __s;
----------------



================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp:20
+
+  std::string str;
+#ifndef TEST_HAS_NO_UNICODE_CHARS
----------------
For completeness please add tests for `std::wstring` and `std::u8string`, both properly guarded.
Note that the `sizeof(wchar_t)` can be 16 or 32-bit.


================
Comment at: libcxx/utils/gdb/libcxx/printers.py:192
 
 class StdStringPrinter(object):
     """Print a std::string."""
----------------
philnik wrote:
> Mordante wrote:
> > 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 ;-)
> Do you know where I would have to look to know what the LLDB pretty printers do?
Unfortunately no. @jingham seems to be the Data formatter code owner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123580