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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with fixed test and passing CI.

Note that this is really tricky. The code looks correct upon inspection, and I think we should catch pretty much any issue via CI. However, if weird stuff starts happening with `std::string` in the upcoming few weeks (when I'm OOO), this should be the number one suspect, and it should be considered fine to revert this in case we suspect `std::string` is broken.



================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp:10
+// <string>
+
+#include <cassert>
----------------
Please add a short comment explaining what you're testing in this test. We already have a `max_size.pass.cpp` file in `test/std`, so how is this one different? The comment should address that.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp:25-27
+#ifdef _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
+
+#  ifdef _LIBCPP_BIG_ENDIAN
----------------
Instead of those `#ifdefs`, I think we should instead be checking for specific platforms/architectures. Otherwise, this test informs us less about the behavior that we expect on specific platforms, and more about what the library itself does. So for example, if we changed the library in a way that broke behavior on a given platform, this test could keep passing as long as we were consistent w.r.t. `_LIBCPP_BIG_ENDIAN` and `_LIBCPP_ABI_ALTERNATE_STRING_LAYOUT`.

So instead, I suggest going for something like

```
#if defined(__APPLE__) && defined(__aarch64__)
// test behavior for alternate string layout
#elif ...
// test behavior for normal string layout
#endif
```

I don't think it should be *too* hard to come up with those `#ifdef`s to satisfy the CI.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/over_max_size.pass.cpp:11
 // XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11}}
+// XFAIL: use_system_cxx_lib && target=arm64{{.+}}-apple-macosx{{11.0|12.0}}
 
----------------
Please add a comment explaining why you're adding this one. Something like

```
// Prior to http://llvm.org/D123580, there was a bug with how the max_size()
// was calculated. That was inlined into some functions in the dylib, which leads
// to failures when running this test against an older system dylib.
// XFAIL: use_system_cxx_lib && target=arm64{{.+}}-apple-macosx{{11.0|12.0}}
```

I don't know why the above `XFAIL` line doesn't have a comment, but one should have been added when we `XFAIL`ed it. Of course you don't need to add one, but that's the spirit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123580