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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked an inline comment as done.
var-const added inline comments.


================
Comment at: libcxx/include/string:2656
     {
-        if (__cap < __n)
+        if (__addr_in_range(*__first))
         {
----------------
var-const wrote:
> var-const wrote:
> > philnik wrote:
> > > 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.
> > @philnik I don't fully understand the case you're describing, can you please give a code example?
> 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).
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149832