[libcxx-commits] [PATCH] D91778: [libc++] [C++20] [P0966] Fix bug PR45368 by correctly implementing P0966: string::reserve should not shrink.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D91778#2410947 <https://reviews.llvm.org/D91778#2410947>, @curdeius wrote:

> I think I understood the problem with failing `Apple back-deployment macosx10.9`... I took a look at `libcxx/utils/ci/run-buildbot` that it invokes. It's not going to work in pre-merge test, because it uses libc++ binaries that it downloads from http://lab.llvm.org:8080/roots/libcxx-roots.tar.gz (obviously not compiled with changes from this patch).
> @ldionne, any idea how this should be addressed?

The usual way of handling these test failures caused by a bug being fixed in the dylib is to mark the tests as `XFAIL` based on the system library being used. The following should work:

  // This test relies on https://llvm.org/PR45368 being fixed, which isn't in
  // older Apple dylibs
  // 
  // XFAIL: with_system_cxx_lib=macosx10.15
  // XFAIL: with_system_cxx_lib=macosx10.14
  // XFAIL: with_system_cxx_lib=macosx10.13
  // XFAIL: with_system_cxx_lib=macosx10.12
  // XFAIL: with_system_cxx_lib=macosx10.11
  // XFAIL: with_system_cxx_lib=macosx10.10
  // XFAIL: with_system_cxx_lib=macosx10.9

Regarding the other failures, they do seem flaky indeed. We need to figure out what's wrong with those modules related tests.



================
Comment at: libcxx/include/string:3268
 void
 basic_string<_CharT, _Traits, _Allocator>::reserve(size_type __res_arg)
 {
----------------
I'd be in favour of renaming this to something like `__requested_capacity`.


================
Comment at: libcxx/include/string:3289
+{
+    size_type __res_arg = __recommend(size());
+    if (__res_arg == capacity()) return;
----------------
Using something like `__target_capacity` here would convey more IMO.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp:29
-    s.reserve();
-    LIBCPP_ASSERT(s.__invariants());
-    assert(s == s0);
----------------
Is there a reason why you're not checking invariants anymore?


================
Comment at: libcxx/www/cxx2a_status.html:266
 
+<p>P0966 was previosly erroneously marked as complete in version 8.0. See <a href="https://bugs.llvm.org/show_bug.cgi?id=45368">Bug 45368</a></p>
 
----------------
Typo!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91778