[libcxx-commits] [PATCH] D149832: [libc++][ranges] Implement the changes to `basic_string` from P1206 (`ranges::to`):

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a subscriber: philnik.
ldionne added inline comments.


================
Comment at: libcxx/include/string:2627
+#if _LIBCPP_STD_VER >= 17
+  if constexpr (
+#else
----------------
var-const wrote:
> 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.
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.


================
Comment at: libcxx/include/string:2656
     {
-        if (__cap < __n)
+        if (__addr_in_range(*__first))
         {
----------------
var-const wrote:
> 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)
I think I agree with this, @philnik does that make sense to you (you are pretty familiar with string).


================
Comment at: libcxx/include/string:1446
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr iterator insert_range(const_iterator __position, _Range&& __range) {
+      if constexpr (ranges::forward_range<_Range> || ranges::sized_range<_Range>) {
----------------
var-const wrote:
> ldionne wrote:
> > `__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.
> `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?
No, this makes sense to me now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149832