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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


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


================
Comment at: libcxx/include/string:2647-2651
+#if _LIBCPP_STD_VER >= 17
+    static_assert(__string_is_trivial_iterator<_Iterator>::value);
+#else
+    _LIBCPP_ASSERT(__string_is_trivial_iterator<_Iterator>::value);
+#endif
----------------
Why do you have this condition here? We have `static_assert`s all the way back to C++03.


================
Comment at: libcxx/include/string:2656
     {
-        if (__cap < __n)
+        if (__addr_in_range(*__first))
         {
----------------
ldionne wrote:
> 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).
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149832