Page MenuHomePhabricator

[libc++] Implement P2438R2 (std::string::substr() &&)
Needs ReviewPublic

Authored by philnik on Aug 11 2022, 4:31 AM.

Details

Reviewers
ldionne
Mordante
var-const
fsb4000
avogelsgesang
Group Reviewers
Restricted Project
Summary

This doesn't affect our ABI because std::string::substr() isn't in the dylib and the mangling of substr() const and substr() const& are different.

Diff Detail

Unit TestsFailed

TimeTest
3,280 mslibcxx CI AArch64 -fno-exceptions > llvm-libc++-shared-cfg-in.std/strings/basic_string/string_cons::substr_rvalue.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/local/bin/c++ /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp --target=aarch64-linux-gnu -nostdinc++ -I /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/include/c++/v1 -I /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fno-exceptions -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -Wl,-rpath,/home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/lib -lc++ -pthread -o /home/tcwg-buildbot/worker/linaro-aarch64-libcxx-02/llvm-project/libcxx-ci/build/aarch64-noexceptions/test/std/strings/basic.string/string.cons/Output/substr_rvalue.pass.cpp.dir/t.tmp.exe
3,910 mslibcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.libcxx/input_output/filesystems/class_path/path_member/path_native_obs::string_alloc.pass.cpp
Script: -- : 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/libcxx/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk --target=x86_64-apple-macosx10.15 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -nostdlib++ -L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/lib -lc++ -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/test/libcxx/input.output/filesystems/class.path/path.member/path.native.obs/Output/string_alloc.pass.cpp.dir/t.tmp.exe
1,890 mslibcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.std/containers/associative/map/map_access::index_key.pass.cpp
Script: -- : 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/std/containers/associative/map/map.access/index_key.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk --target=x86_64-apple-macosx10.15 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -nostdlib++ -L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/lib -lc++ -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/test/std/containers/associative/map/map.access/Output/index_key.pass.cpp.dir/t.tmp.exe
2,009 mslibcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.std/containers/associative/map/map_modifiers::insert_and_emplace_allocator_requirements.pass.cpp
Script: -- : 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/std/containers/associative/map/map.modifiers/insert_and_emplace_allocator_requirements.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk --target=x86_64-apple-macosx10.15 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -nostdlib++ -L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/lib -lc++ -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/test/std/containers/associative/map/map.modifiers/Output/insert_and_emplace_allocator_requirements.pass.cpp.dir/t.tmp.exe
1,610 mslibcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.std/containers/associative/set::insert_and_emplace_allocator_requirements.pass.cpp
Script: -- : 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/std/containers/associative/set/insert_and_emplace_allocator_requirements.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk --target=x86_64-apple-macosx10.15 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -nostdlib++ -L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/lib -lc++ -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/test/std/containers/associative/set/Output/insert_and_emplace_allocator_requirements.pass.cpp.dir/t.tmp.exe
View Full Test Results (126 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
philnik added inline comments.Aug 17 2022, 2:55 AM
libcxx/include/string
113

what exactly are you asking for?

891–892

No, this constructor is part of the ABI.

899

Yes, I think so. I couldn't find any comprehensive tests for the debug mode though. Do you know if we have any/where they are?

904

Yes, but that allows the compiler to always remove the comparison.

907

Do you mean if it's optimized out when the compiler knows that __pos == 0? Or do you mean it's fine as-is, since this scenario is optimized in memmove?

1380

I don't know why we have the non-conforming behaviour. I just didn't want to change the behaviour in this patch. I've filed https://llvm.org/PR57190.

libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
186

As usual, constexpr step limit.

Mordante added inline comments.Aug 17 2022, 9:18 AM
libcxx/include/string
113

To split the line in two and align the since C++23 with the other since comments.

907

I'm fine with this as is, so leave it to memmove. It gives some overhead by calling a function, but I don't think that's a big deal.

1380

I still would like a comment in the code mentioning this is non-standard behaviour and a link to the bug report.
That way when somebody sees this and wonders why it's clear this was done intentionally.

libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
186

maybe add a comment, however I really love to increment the limit; but you're already seen that patch ;-)

philnik updated this revision to Diff 453632.Aug 18 2022, 5:32 AM
philnik marked 9 inline comments as done.
  • Address comments

In general LGTM from my point of view, however I see all MacOS CI runs failing so I expect you have more things to do.

Can you have a look at this question too before I approve?
Looking at the paper I also noticed 4.5. Concerns on ABI stability, did you verify whether this isn't an issue for us?

var-const requested changes to this revision.Aug 18 2022, 3:35 PM
var-const added inline comments.
libcxx/include/string
268–270

Using a single line is consistent with other entries in the synopsis and makes it obvious to which function the comment refers to (whereas for the two-line version, it could refer to either the preceding or the following line).

Sorry, looks like I misread the original comment -- please disregard.

891–892

I'm a little surprised this constructor is not conditionally noexcept. Do you know if this is deliberate or an omission in the paper?

899

I suspect we don't have those tests, but let's confirm with @ldionne.

905

Can you confirm that the is_always_equal check optimizes away even without using if constexpr?

906

Hmm, the move constructor instead does this:

if (__libcpp_is_constant_evaluated()) {
    __zero();
    __r_.first().__l = __str.__r_.first().__l;
} else {
    __r_.first().__r = __str.__r_.first().__r;
}

Your version is simpler. Is there a functional difference? If these are equivalent, I think we should simplify the move constructor as well (not in this patch). If not equivalent, is there a reason why the new constructor can use a simple assignment without the conditional logic?

909

Consider a comment like Move the given string then shrink it in place.

909

Reevaluating data() calls to_address() and is_long() repeatedly. Would it make sense to store the result in an intermediate variable?

913

Consider adding a comment like Perform a copy because allocators have different types.

916

The move constructor also performs

if (__is_long())
    std::__debug_db_swap(this, &__str);

I presume this is necessary here as well -- @ldionne would know more.

1379

+1 to @Mordante's comment re. ABI stability. This should also be documented in the commit message (either why the ABI break doesn't happen in our case, or what we are doing to mitigate it).

1380

Is the proposed resolution of LWG3752 to pass the allocator? I couldn't find the discussion it refers to, but because it says "we will fix it", I take it to mean that the current behavior, i.e., passing __alloc() should be changed, and the paper merely asks for confirmation.

This revision now requires changes to proceed.Aug 18 2022, 3:35 PM
var-const added inline comments.Aug 19 2022, 12:01 AM
libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
36

Optional: consider moving the checks into a helper function to cut down on boilerplate.

54

Can we do some equivalent of DisableAllocationGuard in this test as well?

62

I'd split the normal case and the exceptional case in two functions. These are very different concerns, and I find the current triple-check (if pos is not valid, and it's not constant-evaluated, and exceptions are enabled) a little overwhelming. Making the expected parameter double as essentially a boolean also doesn't feel very clean. This would also be clearer at the call site (I'd group all normal cases and all exception cases together, i.e. a bunch of test_string_pos, test_string_pos_n, etc., followed by a bunch of test_string_pos_exception, test_string_pos_n_exception, etc.

I know it will increase the number of test_string_* functions even more, but I think it's the lesser evil. The amount of code won't increase substantially, and the smaller functions will be easier to read.

119

Question (not for this patch): do we still need these macros even in the latest language modes, or can this now be achieved by regular code?

libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp
27

The implementation is recommended to avoid allocation, though, and I think we want to check that we follow that.

29

orig_string_copy seems unused?

30

What's the benefit of calculating rlen, given that the str == expected check would also compare lengths? If it's necessary to check the length explicitly for some reason, I think it should be an argument to this function.

33

In the constructor test we don't check __invariants() on str, can you make this consistent?

libcxx/test/support/count_new.h
439–443

Why is this a no-op in consteval mode?

453

Why are the constexpr language versions different between member functions?

philnik updated this revision to Diff 454077.Aug 19 2022, 12:14 PM
philnik marked 13 inline comments as done.
  • Address comments
libcxx/include/string
891–892

Why are you surprised this isn't conditionally noexcept? Only the default constructor is conditionally noexcept, and the allocator and move constructors are always noexcept. Nothing else is.

905

Do you mean something like this?

906

I don't think there is any difference. I'll make a follow-up.

909

If the compiler can't optimize it IMO we should give the compiler hints on data() instead of decreasing the readability of the code.

909

I think this comment is more misleading than helpful. _Traits::move() and __set_size() seem to me quite intuitive. shrink in place sounds a lot like we shrink_to_fit() here, which we definitely don't want to do.

913

Would it make more sense to rename __init to something like __init_from_char_array to make it obvious that this path copies the array?

1379

It's simple actually. The mangling for the functions is different (https://godbolt.org/z/P8dfdYPs7) and we don't have it in our ABI.

1380

There is no officially proposed solution (as you can see). If you're asking if that's what the standard says, yes.

libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
54

Not really. Here the constructor sometimes has to allocate.

62

I really wouldn't group normal and exception cases together. Currently you can easily see that the exceptions cases are testing the edge cases, which wouldn't be the case anymore when moving them somewhere else. IMO it's also cleaner to always call the same function for testing the same function in this case. Having the "exception" at the end makes it quite obvious which are throwing while making the rest of the arguments easily comparable.

119

AFAIK we still don't have any standard conversion functions, so I guess we still need the macro.

libcxx/test/std/strings/basic.string/string.ops/string_substr/substr_rvalue.pass.cpp
27

Again, we can't do that because the constructor might have to allocate.

30

There isn't any. That was a leftover from a previous iteration.

libcxx/test/support/count_new.h
439–443

Because we use global variables here.

453

Destructors can only be constexpr in C++20 and later.

Mordante accepted this revision as: Mordante.Aug 20 2022, 7:39 AM

I still see a lot of open comments of @var-const and CI failures, but I've no additional issues.
So I approve this patch to make that clear and leave the final approval to @var-const.

libcxx/include/string
1379

Thanks for the explanation and +1 adding this information to the commit message.

1380

For me it's fine to keep this since we already have this behaviour in our original substr code. So I don't mind waiting for the final resolution of LWG3752 before changing the code.

var-const added inline comments.Aug 23 2022, 5:54 PM
libcxx/include/string
891–892

To be clear, I'm surprised that this isn't noexcept in some way (I just think it would have to be conditionally, but I didn't give much thought to it), since it's similar to the move constructor in many ways.

905

The interesting part, IMO, is that it depends on the template type, but I presume it won't make a difference to the compiler.

909

Personally, I find using a variable more readable (for me, the repeated function calls are "noisy" and add extra cognitive overhead of thinking whether the function result might change upon subsequent calls). Also, performance in debug mode is a common concern among users.

913

I'd still prefer a comment. Even __init_from_char_array doesn't make it obvious that a copy is performed, and the part about allocators having different types is helpful as well.

1379

Please add that explanation to the patch / commit description.

1380

In that case, I feel we shouldn't pass __alloc for the rvalue overload -- yes, it would be inconsistent, but I don't think we should introduce more divergence and have more stuff to fix later. This is definitely something to check with @ldionne.

libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
36

Friendly ping (feel free to push back).

62

Currently you can easily see that the exceptions cases are testing the edge cases, which wouldn't be the case anymore when moving them somewhere else.

I don't see how it would be less readable. The current state is

test_string_pos<S>(STR(""), 0, STR(""));
test_string_pos<S>(STR(""), 1, STR("exception"));
test_string_pos<S>(STR("Banane"), 1, STR("anane"));
test_string_pos<S>(STR("Banane"), 6, STR(""));
test_string_pos<S>(STR("Banane"), 7, STR("exception"));
test_string_pos<S>(STR("long long string so no SSO"), 0, STR("long long string so no SSO"));

I would prefer

test_string_pos<S>(STR(""), 0, STR(""));
test_string_pos<S>(STR("Banane"), 1, STR("anane"));
test_string_pos<S>(STR("Banane"), 6, STR(""));
test_string_pos<S>(STR("long long string so no SSO"), 0, STR("long long string so no SSO"));

test_string_pos_exception<S>(STR(""), 1);
test_string_pos_exception<S>(STR("Banane"), 7);

Which I think makes it even more obvious that now we're testing corner cases. Alternatively, keeping the same order is fine too:

test_string_pos<S>(STR(""), 0, STR(""));
test_string_pos_exception<S>(STR(""), 1);
test_string_pos<S>(STR("Banane"), 1, STR("anane"));
test_string_pos<S>(STR("Banane"), 6, STR(""));
test_string_pos_exception<S>(STR("Banane"), 7);
test_string_pos<S>(STR("long long string so no SSO"), 0, STR("long long string so no SSO"));

Personally, I think this would be more readable, or at the very least no less readable, while making the existing issues (overcomplicated condition in test_string_pos, magic value in expected) go away.

Mordante added inline comments.Aug 24 2022, 10:38 AM
libcxx/include/string
1380

Since @ldionne mentioned on Discord LWG-3752 probably will be closed as NAD I agree more with @var-const.(I had expected LWG to mandate the current MSVC STL and libc++ implementation.)

philnik edited the summary of this revision. (Show Details)Tue, Sep 6, 10:30 AM
philnik marked 4 inline comments as done.Tue, Sep 6, 10:47 AM
philnik added inline comments.
libcxx/include/string
913

I'm not sure if you are just a bit sloppy with the wording or if you misunderstand something. The allocators always have the same type, but the allocators may not compare equal.

1380

I'd still rather do it in a separate patch, because it is a significant behaviour change that has nothing to do with P2438. The overloads should not have different behaviour IMO.

libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
36

In my experience having the checks in it's own function just makes the test harder to debug, so I'd rather keep them duplicated.

62

I prefer the first one, so I guess we just disagree here. IMO the second one makes it a lot less obvious that we're checking corner cases, because I can't immediately see that STR("Banane"), 7 is the exact case where it should throw, which I do immediately see if the line before was STR("Banane"), 6. The last one seems just messy to me, but that's nothing you can argue much about.

ldionne accepted this revision as: ldionne.Thu, Sep 8, 9:14 AM

The implementation LGTM, I have a few comments.

libcxx/include/string
899

I don't think we have extremely comprehensive tests for the debug mode for string, but we do have e.g. libcxx/test/libcxx/strings/basic.string/string.iterators/debug.iterator.index.pass.cpp & friends in the same directory.

I think we do want a simple test to make sure that the added constructor works properly with the debug mode. Concretely, I would imagine something like:

std::string s = "hello world";
std::string s2(std::move(s), 0, 5); // "hello"
std::string::iterator i = s2.begin();
assert(i[0] == 'h');
TEST_LIBCPP_ASSERT_FAILURE(i[5], "Attempted to subscript an iterator outside its valid range");
913

I think what @var-const meant was:

// Perform a copy because the allocators are not compatible.

The allocators have the same type, they just compare unequal, which in alllocator-land means that the memory allocated by one can't be freed by the other.

916

Test needed!

1380

I agree with @philnik here: let's implement this consistently with our current substr behavior, but then fix both substr behaviors to be standards conforming per LWG3752's expected resolution ASAP.

Alternatively, we could implement LWG3752's expected resolution first, and then land this patch consistently with the fixed substr(). I don't care in what order this happens, as long as we don't ship another release without LWG3752's resolution. But I think we should really avoid introducing a difference between our two substr() implementations.

libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
29–51

Instead, I would suggest the following:

template <class S>
constexpr void test_string_pos(S orig, typename S::size_type pos, S expected) {
#ifdef _LIBCPP_VERSION
  DisableAllocationGuard g;
#endif
  S substr(std::move(orig), pos);
  LIBCPP_ASSERT(orig.__invariants());
  LIBCPP_ASSERT(orig.empty());
  LIBCPP_ASSERT(substr.__invariants());
  assert(substr == expected);
}

constexpr struct should_throw_exception_t { } should_throw_exception{};

template <class S>
constexpr void test_string_pos(S orig, typename S::size_type pos, should_throw_exception_t) {
#ifndef TEST_HAS_NO_EXCEPTIONS
  if (!std::is_constant_evaluated()) {
    try {
      [[maybe_unused]] S substr = S(std::move(orig), pos);
      assert(false);
    } catch (const std::out_of_range&) {

    }
  }
#endif
}

We could also use a differently-named function, but you seemed to like the naming consistency at the callsite IIUC. Basically, what I care about is that we remove this logic from the test since it's so easy to remove, and also since those two functions do entirely different things.

philnik updated this revision to Diff 461369.Mon, Sep 19, 2:38 PM
philnik marked 11 inline comments as done.
  • Rebased
  • Address comments
var-const accepted this revision as: var-const.Tue, Sep 20, 4:05 PM
var-const added inline comments.
libcxx/include/string
909

Ping.

913

Yeah, I'm not sure why I was thinking about types when I was typing this. Current wording LGTM.

libcxx/test/std/strings/basic.string/string.cons/substr_rvalue.pass.cpp
62

I think the function name having _exception conveys that this tests corner cases a lot clearer than anything else could. On the other hand, there isn't anything that semantically connects the 6 and 7 test cases together -- this is at best very brittle (in the future, it's easy for someone to rearrange these test cases, insert a new test case between them, etc.) and requires reading the code closely and comparing test cases across the lines.