[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 added inline comments.


================
Comment at: libcxx/include/__iterator/common_iterator.h:45
     constexpr explicit __proxy(iter_reference_t<_Iter>&& __x)
-      : __value(_VSTD::move(__x)) {}
+      : __value_(static_cast<iter_reference_t<_Iter>&&>(__x)) {}
 
----------------
ldionne wrote:
> I don't understand why we're changing this one.
Somewhat cascading drive-by. I changed the `forward` on line 59; then noticed that `__value` should be `__value_` down there; then noticed that the same thing applied up here; and finally, noticed that `__proxy` and `__postfix_proxy` were actually doing the same thing, except that `__postfix_proxy` was misusing `forward` to do its `static_cast<X&&>`, while `__proxy` was "misusing" `move` (a trick that wouldn't even have worked in general, as your blog post says — but worked here because the programmer happened to know something about `iter_reference_t<_Iter>`, but then they felt guilty about the cleverness and left a comment explaining their logic. //BUT,// if we remove the cleverness, then we can also remove the guilty comment. The new line 45 is correct by definition, even if `iter_reference_t<_Iter>` //were// someday to become a reference type.

(That said, I'm now wondering why both of these types aren't simply aggregates!)


================
Comment at: libcxx/include/__memory/unique_ptr.h:210
   unique_ptr(unique_ptr&& __u) _NOEXCEPT
-      : __ptr_(__u.release(), _VSTD::forward<deleter_type>(__u.get_deleter())) {
+      : __ptr_(__u.release(), static_cast<deleter_type&&>(__u.get_deleter())) {
   }
----------------
ldionne wrote:
> Can the deleter be a reference? If not, I don't understand why we're not `std::move`ing here (and below) instead.
Yes, `deleter_type` explicitly //can// be an lvalue reference type.


================
Comment at: libcxx/include/__utility/pair.h:224
                     is_nothrow_constructible<second_type, _U2&&>::value))
-        : first(_VSTD::forward<_U1>(__p.first)), second(_VSTD::forward<_U2>(__p.second)) {}
+        : first(static_cast<_U1&&>(__p.first)), second(static_cast<_U2&&>(__p.second)) {}
 
----------------
ldionne wrote:
> `std::move(__p).first`?
That would be correct AFAICT (per your blog post), but is it really more readable, or will it lead to people asking (as in your blog post) why not `std::move(__p.first)` or why isn't that a double-move bug? My attitude is, if we want to cast it to precisely the type `_U1&&`, for whatever metaprogrammy reasons, we should just //say// that, instead of trying to jenga a cast out of some clever call(s) to `move` or `forward`.

IOW, arguably, `static_cast<X&&>` means "Look out! This is subtle for some reason!", whereas `std::move` doesn't set off any alarm bells. And I'm claiming that if it matters for correctness whether you've written `std::move(x).y` or `std::move(x.y)`, then //that is subtle by definition//, and some alarm bells are warranted.


================
Comment at: libcxx/include/tuple:1591
                      __tuple_indices<_I1...>, __tuple_indices<_I2...>)
-    :  first(_VSTD::forward<_Args1>(_VSTD::get<_I1>( __first_args))...),
-      second(_VSTD::forward<_Args2>(_VSTD::get<_I2>(__second_args))...)
+    :  first(static_cast<_Args1&&>(_VSTD::get<_I1>(__first_args))...),
+      second(static_cast<_Args2&&>(_VSTD::get<_I2>(__second_args))...)
----------------
ldionne wrote:
> Since the standard says this: http://eel.is/c++draft/pairs#pair-19, I think I'd rather leave it as-is. It would better if the Standard said `static_cast`, I think, but oh well.
TBF, it doesn't use this exact expression; it says it in English.
> Initializes first with arguments of types Args1... obtained by forwarding the elements of first_­args and initializes second with arguments of types Args2... obtained by forwarding the elements of second_­args. (Here, forwarding an element x of type U within a tuple object means calling std​::​forward<U>(x).) 

Also, maybe you'll change your mind when you see that this is not actually the ctor `pair(piecewise_construct_t, tuple<Args1...>, tuple<Args2...>)` defined in the standard. This is `pair(piecewise_construct_t, tuple<_Args1...>&, tuple<_Args2...>&, __tuple_indices<_I1...>, __tuple_indices<_I2...>)`. So we've already messed with the types of `first_args` and `second_args` by ref-qualifying them. (Mind you, that doesn't change their value categories as far as the paragraph above is concerned.)

I strongly prefer not to use `std::forward` here, because `std::forward<_Args1>(std::get<_I1>(__first_args))` is **not equivalent** to `static_cast<decltype(std::get<_I1>(__first_args))&&>(std::get<_I1>(__first_args))`, which means I can't (locally) turn it into `_LIBCPP_FWD(std::get<_I1>(__first_args))`. I want to permanently burninate any use of `std::forward<X>(y)` that can't be turned into `_LIBCPP_FWD(y)`, because (by my definition at least) that is **not forwarding**.

Per your blog post, this is another place where we //could// use `get<I>(move(x))`:
```
    :  first(std::get<_I1>(std::move(__first_args))...),
```
Physically correct, readability-wise-scary to me. But still, if you forced me to choose between `forward<X>(get<I>` or `get<I>(move(`, I would choose `get<I>(move(`. Because `get<I>(move(` is subtle, but `forward<X>(get<I>(` is just a flat-out lie AFAIC. It's not forwarding!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120998