[libc++][ranges] Implement the changes to `basic_string` from P1206 (`ranges::to`):

[var-const]

Summary

• add the from_range_t constructors and the related deduction guides;
• add the insert_range/assign_range/etc. member functions.

(Note: this patch is split from https://reviews.llvm.org/D142335)

[var-const]

Address feedback

[var-const]

libcxx/include/string

1296

Original comment by @ldionne

This one can be more efficient. If we have an input range, then we do need to make a temporary string and append it. But if we have e.g. a forward_range or a sized_range, we don't need to make a copy of the input range, and I think we should really avoid doing it. I think simply using insert_range(end(), ...) here would solve that problem?

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp

27

Original comment by @ldionne

You are also missing a SFINAE test for the constraints of the method.

56

Original comment by @ldionne

// - input range: empty / one-element / middle-sized / longer than SSO / longer than current string capacity.

863

Original comment by @ldionne

We don't seem to be testing that the range is taken as Rng&&, are we?

[var-const]

libcxx/include/string

1296

Done (here and in assign_range).

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/replace_with_range.pass.cpp

56

Added a few test cases suffixed with _LongerThanCapacityRange.

[var-const]

Fix the CI.

[var-const]

Fix CI

[ldionne]

libcxx/include/string

1369–1371

We should refactor this so that __assign_with_size actually assigns with size, so we don't have to compute __n = 0 when we don't actually need it anywhere. That's too confusing, it seems like we're assigning with size 0 when !__string_is_trivial_iterator but in reality __assign_with_size simply ignored the size we passed in.

1415

__string_is_trivial_iterator was introduced around D98573. Do we have the same issue in the insert_range you're adding? Why do we not need to check for __string_is_trivial_iterator here?

I suspect there is an exception-safety issue hidden in the current version of the code, in which case we should add a test case similar to what was added in D98573 but for the new methods in this patch.

libcxx/test/std/strings/basic.string/string.cons/from_range.pass.cpp

21–22

Above includes (here and elsewhere).

libcxx/test/std/strings/basic.string/string.modifiers/string_append/append_range.pass.cpp

20

Here and in other tests, I would inline those functions into main() and instead do:

// plain english comment explaining what you test
{
  // test content
}

This is what we try to do elsewhere and a plain text comment allows for a more precise description of what's going on than a function name -- I'm not 100% sure what's being tested by just reading test_string_append_range_return_value.

libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_string_size_size.pass.cpp

2 ↗

(On Diff #523238)

I think this change is accidental.

[var-const]

Partially address feedback and rebase.

libcxx/include/string

1369–1371

I gave it a shot, but very open to suggestions.

Thinking more about it, it's not really "assign with size" since it only applies to a subset of cases where we know the size:

• first, the input iterators have to be "trivial";
• second, if the input range is a subset of the string itself, then we need to have enough capacity to avoid reallocating (or the input iterators will end up being invalidated).

These conditions are somewhat messy, so to avoid duplicating the check in two different places (assign and assign_range), I made it a try function that returns false if it fails to do the job (so that the caller can fall back to assign_with_sentinel). This also avoids the question of whether try_assign_trivial should assert that its preconditions are met or just trust the caller. Please let me know what you think!

[var-const]

libcxx/include/string

2545

I'm very open to reverting this, perhaps it's overkill. By the way, do we have any existing precedent for cases where we would want to use if constexpr but can't due to pre-C++17 language modes? I didn't find an obvious macro to use in __config, but perhaps I missed something.

2550–2551

Note: this is an inversion of the original "positive" condition ("if condition, proceed" now inverted to "if not condition, return early"):

if (__cap >= __n || !__addr_in_range(*__first)))

Unless I made a mistake and the rewrite is not equivalent, I don't really see how this case could happen. If __first is __addr_in_range, i.e., an iterator pointing at a character within the string itself, it seems that the input range cannot be longer than the size of the string, which cannot be less than the capacity. I don't think it's possible for the end iterator to point to some characters past the end of the string without UB. Perhaps I'm missing something? FWIW, no tests fail if I remove this early return.

(If we could remove this condition, it would simplify the function so that it no longer has to return a boolean)

[var-const]

libcxx/include/string

1415

insert_range delegates to __insert_with_size which falls back to constructing a temporary string if the iterator is not trivial. I don't think there should be an exception safety issue, but the code is not symmetrical with assign_range now. Should I introduce something like __insert_with_sentinel to mirror the other function?

[ldionne]

libcxx/include/string

1415

No, this makes sense to me now.

2545

Yeah, I think we should keep the previous code here. I don't think we have something to turn into a if constexpr in C++ >= 17 but if we did, then it would 100% make sense to use it here. I just don't think the #if is worth it.

2550–2551

I think I agree with this, @philnik does that make sense to you (you are pretty familiar with string).

[philnik]

libcxx/include/string

2545

I think we have quite a few places where if constexpr would be nice but not necessary, since the code will compile without the constexpr.

2550–2551

Users are allowed to inspect the object, so this will be the case if a user gives us a pointer to the value representation of this string. The question is just whether we want to bother supporting this, since about two people on this planet will actually do that.

2565–2569

Why do you have this condition here? We have static_asserts all the way back to C++03.

[var-const]

Address feedback and rebase.

libcxx/include/string

2550–2551

@philnik I don't fully understand the case you're describing, can you please give a code example?

2565–2569

I think if I'm not using if constexpr in the caller, this function will always end up being instantiated, whether it's called or not, so I can't use static_assert. I reverted the conditional if constexpr, so this is no longer applicable.

[var-const]

libcxx/include/string

2550–2551

Synced offline, it's basically:

std::string str("abc");
auto* ptr = reinterpret_cast<char*>(&str);
str.assign(ptr, ptr + sizeof(str));

IIUC, there's no undefined behavior here -- growing capacity will change the byte representation of the string but won't make it invalid. I think we don't need to look for that case (and we already don't in the overload of assign that takes a pointer and a size).

[var-const]

__try_assign_trivial -> __assign_trivial.

[var-const]

libcxx/include/string

2550–2551

Simplified __try_assign_trivial to just __assign_trivial (so that now it doesn't return anything).

If I had to guess, the patch that added the check added a similar check to append (where it makes sense) and perhaps just duplicated it in assign without realizing that this case doesn't apply.

[var-const]

Rebase

[ldionne]

libcxx/include/string

1514

I might have asked for that already, but do you have tests that these constraints are right, i.e. this overload doesn't match if you pass something that's not _ContainerCompatibleRange?

2558

You'll need to rebase on top of main and this should change to _LIBCPP_ASSERT_UNCATEGORIZED.

libcxx/test/std/strings/basic.string/string.cons/from_range.pass.cpp

13

This is marked as constexpr.

112

You don't seem to be testing this inside constexpr, but the constructor is definitely marked as constexpr. The same comment seems to apply to all tests.

libcxx/test/std/strings/basic.string/string.modifiers/string_append/append_range.pass.cpp

63–64

Can't we just inline this function here in a local scope? Applies to other tests as well.

[var-const]

Address feedback

libcxx/include/string

1514

Yes, see constexpr bool test_constraints_replace_with_range() in replace_with_range.pass.cpp.

libcxx/test/std/strings/basic.string/string.cons/from_range.pass.cpp

112

Thanks! Sorry, somehow I missed that.