[libcxx-commits] [PATCH] D98109: [libcxx] Make path::absolute return paths with separators in preferred form
Martin Storsjö via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Mar 6 09:18:29 PST 2021
mstorsjo added inline comments.
Comment at: libcxx/src/filesystem/operations.cpp:701
- return __do_absolute(p, &cwd, ec);
+ return __do_absolute(p, &cwd, ec).make_preferred();
> 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();
Not sure if that's better or worse wrt optimizations, but it's more understandable.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits