[libcxx-commits] [PATCH] D116621: [libc++][P2321R2] Add const overloads to tuple swap, construction and assignment

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

I would like to test this against Google's source. I'll try to get that done by early this week. Please do not commit this until then.
Tuple is so sensitive that changing whitespace breaks users.



================
Comment at: libcxx/include/tuple:754
             _Lazy<_And,
-                _Not<is_convertible<const tuple<_Up>&, _Tp> >...,
-                _Not<is_constructible<_Tp, const tuple<_Up>&> >...
+                  _Not<is_convertible<__maybe_const<_Const, tuple<_Up>>&, _Tp> >...,
+                  _Not<is_constructible<_Tp, __maybe_const<_Const, tuple<_Up>>&> >...
----------------
What's `__maybe_const`? Should it be in this patch?


================
Comment at: libcxx/include/tuple:848
     struct _EnableImplicitCopyFromPair : _And<
-        is_constructible<_FirstType<_DependentTp...>, const _Up1&>,
-        is_constructible<_SecondType<_DependentTp...>, const _Up2&>,
----------------
These bits of SFINAE were written very specifically because tuple is a very weird beast.  

I'm surprised we don't have a test in the test suite that breaks after changing this, because I thought I added one.

There are certain SFINAE loops that crop up where checking these properties in the specific order is/was important.



================
Comment at: libcxx/include/tuple:1344
+swap(const tuple<_Tp...>& __lhs, const tuple<_Tp...>& __rhs)
+        noexcept(__all<is_nothrow_swappable_v<const _Tp&>...>::value) {
+    __lhs.swap(__rhs);
----------------
What's with the reference here


================
Comment at: libcxx/include/type_traits:470
 
+struct __nat
+{
----------------
`__nat` is an awful hack. Let's not user it more.


================
Comment at: libcxx/include/type_traits:506-509
+  template <class...>
+  using _FirstImpl = __nat;
+  template <class...>
+  using _SecondImpl = __nat;
----------------
philnik wrote:
> Quuxplusone wrote:
> > Uh...? This completely changes the meaning of e.g. `_SecondType<int>` from "ill-formed" to "well-formed `__nat`." There's no way that's safe.
> `_FirstType` and `_SecondType` are exclusively used in `<tuple>` and only in `is_*` contexts. If I'm not mistaken `__nat` should be false in every single case.
I'm with Arthur, this should not succeed. This needs to be substitution failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116621