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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Regarding `std::get<I>(std::move(tuple))`, this provides a potentially relevant formal definition of when it's "okay" to do it: https://ldionne.com/2016/02/17/a-tentative-notion-of-move-independence/. In most places in this patch, my preference would be to use `std::get<_I0>(std::move(__t))`, since IMO that's the clearest form. In several places, we take the argument we're moving out of by value, so IMO using `std::move` on it makes a lot of sense.

Also -- you replaced all those places and a test failed for each of them? That's nice.



================
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)) {}
 
----------------
I don't understand why we're changing this one.


================
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())) {
   }
----------------
Can the deleter be a reference? If not, I don't understand why we're not `std::move`ing here (and below) instead.


================
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)) {}
 
----------------
`std::move(__p).first`?


================
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))...)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120998