[libcxx-commits] [PATCH] D98109: [libcxx] Make path::absolute return paths with separators in preferred form

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:701
   path cwd;
-  return __do_absolute(p, &cwd, ec);
+  return __do_absolute(p, &cwd, ec).make_preferred();
 }
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > Your friendly neighborhood `return x` expert says: Yikes! Remember that `make_preferred()` modifies the string in place (which is OK, and I checked that it's efficient) but then returns //an lvalue reference to the path//, not a copy of the path!
> > 
> > So the old code was RVO'ing the prvalue result of `__do_absolute`. The new code, on Windows //and even on POSIX where `make_preferred` is a no-op//, is doing a //copy-construct// from the lvalue result of `make_preferred()` into the return slot.
> > 
> > Please, at a minimum, change to use `return cwd;`.
> > If you would like to do larger surgery on `__do_absolute`, to make it more efficient, that would be awesome and I'd definitely take a look at it. (Right now I don't even understand why it takes `cwd` at all; it seems like we do that so that `__canonical` can throw a `filesystem_error` with `path2=cwd`, but that's [not even what cppreference says should happen](https://en.cppreference.com/w/cpp/filesystem/canonical) so I don't know why we do that. Some digging will be required by whoever takes this on.)
> I'm sorry, I didn't follow entirely what you suggest to change, can you clarify? We're not returning `cwd`. Returning with `make_preferred()` is a bit obscure, agreed - the alternative way of expressing the same would be:
> ```
> path ret = __do_absolute();
> ret.make_preferred();
> return ret;
> ```
> Not sure if that's better or worse wrt optimizations, but it's more understandable.
> can you clarify? We're not returning `cwd`.

Oops, right. The thing you wrote, `path ret = __do_absolute(); ret.make_preferred(); return ret;`, is indeed what I meant to say.

It //is// strictly better w.r.t. optimizations; it will do move-construct on paper, and move-elision (RVO) in practice. The code I objected to would have done a full copy-construct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98109