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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo 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();
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98109