[libcxx-commits] [PATCH] D120998: [libc++] Avoid using std::forward on things that aren't forwarding references. NFCI.

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

Quuxplusone created this revision.
Quuxplusone added reviewers: ldionne, Mordante, libc++, philnik, var-const.
Quuxplusone added a project: libc++.
Herald added a project: All.
Quuxplusone requested review of this revision.
Herald added a subscriber: libcxx-commits.
Herald added 1 blocking reviewer(s): libc++.

Inspired by the recent Discord discussion on how to (speed up|stop generating debug symbols for) `std::forward` and `std::move`, I decided to investigate what happens if we mechanically replace all our `std::forward<T>(t)` with `static_cast<decltype(t)>(t)`. In the process, I found these few places where we are using the `std::forward` syntax, but in fact we aren't forwarding anything — we're just using it as a more expensive/verbose static_cast. Regardless of what we do about `std::forward` itself, I'd like to adjust these places to reflect the fact that they're not forwarding.

In a few places you'll wonder why we do `X& f(); ~~~ static_cast<X&&>(f())` instead of just `X& f(); ~~~ std::move(f())`. It's because sometimes `X` itself is an lvalue reference type, so moving-out-of its referent would be wrong, but static-casting to `X&&` will be a no-op.

In a few places in `pair` and `tuple`, one might be tempted to argue that we kinda //are// forwarding, from one member of a tuple/pair. I say sure, arguably; but IMNSHO it is vastly //clearer// to say `static_cast<SpecificType&&>(std::get<I>(t))` than `std::forward<SpecificType>(std::get<I>(t))` (when `t` is an lvalue reference to a tuple). Notice that in these cases, it would //not// be correct to write either `static_cast<decltype(std::get<I>(t))>(std::get<I>(t))` or just `std::get<I>(t)`; and also (per above) it wouldn't be correct to write `std::move(std::get<I>(t))`. I'm not sure if it would be correct to write `std::get<I>(std::move(t))` in some cases, but I don't think that's clear either (and @Mordante has recently complained about apparent-use-after-move involving `std::get`, so he's probably happy not to introduce any more such cases).

After this patch, it will (at least for now) be possible to mechanically search-and-replace all `std::forward<X>(y)` into `static_cast<decltype(y)>(y)`, and all the tests will still pass. I'm not saying we should //land// anything crazy like that, but it'll be handy to have the option for benchmarking.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120998

Files:
  libcxx/include/__functional/bind.h
  libcxx/include/__iterator/common_iterator.h
  libcxx/include/__memory/compressed_pair.h
  libcxx/include/__memory/unique_ptr.h
  libcxx/include/__utility/pair.h
  libcxx/include/tuple

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120998.413003.patch
Type: text/x-patch
Size: 11915 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220304/c3300d3a/attachment-0001.bin>