[libcxx-commits] [PATCH] D96523: [libc++] Rewrite the tuple constructors to be strictly Standards conforming

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

Quuxplusone added a comment.

I haven't really tried to understand this code, but if it passes tests, I certainly don't object to it.



================
Comment at: libcxx/docs/ReleaseNotes.rst:57
+  - The extension allowing a tuple to be constructed from an array has been
+    removed.
----------------
IMHO it would be nice to provide code snippets (Godbolt links?) to things that have changed.
IIUC the removed signatures are https://godbolt.org/z/v49qKe
but I don't know what signatures (if any) are being added.


================
Comment at: libcxx/include/tuple:585
+        _BoolConstant<sizeof...(_Tp) >= 1>,
+        _Not<_IsThisTuple<_Up...> >, // extension to allow mis-behaved user constructors
+        is_constructible<_Tp, _Up>...
----------------
ldionne wrote:
> zoecarver wrote:
> > Nit: it might be simpler to just say `sizeof...(_Up) != 1 || is_same<_Up,  tuple>...` (or even `__is_same(_Up..., tuple)`). Then you could get rid of `_IsThisTuple`.
> I agree, I'd like to get rid of `_IsThisTuple` too. I tried the following three alternatives:
> 
> ```
> _Not<is_same<__uncvref_t<_Up>, tuple>...>
> ```
> 
> This doesn't work because we're expanding a pack into `_Not`, which takes a single template parameter.
> 
> ```
> _Not<is_same<__uncvref_t<_Up>..., tuple> >
> ```
> 
> This doesn't work because we're expanding a pack into `is_same`, which takes two parameters exactly.
> 
> ```
> _BoolConstant<!__is_same(__uncvref_t<_Up>..., tuple)>
> ```
> 
> This doesn't work but I don't know why.
> 
> Let me know if you can think of another way to get rid of `_IsThisTuple`.
> 
FWIW, I don't understand what the "extension" is here, nor what "mis-behaved user constructors" we're worried about. But I haven't cared enough to remove that line and re-run the unit tests to find out.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR27684_contains_ref_to_incomplete_type.pass.cpp:17
 
-// Libc++ has to deduce the 'allocator_arg_t' parameter for this constructor
-// as 'AllocArgT'. Previously libc++ has tried to support tags derived from
-// 'allocator_arg_t' by using 'is_base_of<AllocArgT, allocator_arg_t>'.
-// However this breaks whenever a 2-tuple contains a reference to an incomplete
-// type as its first parameter. See PR27684.
+// Regression test for PR27684.
 
----------------
`// See https://llvm.org/PR27684` — as long as you're normalizing comments here.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/UTypes.pass.cpp:40
+// Make sure the _Up... constructor SFINAEs out when some types are not
+// explicitly initialized.
+void test_sfinae_missing_elements()
----------------
Nit: `when there are fewer initializers than elements` or `fewer constructor arguments than tuple elements`.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/alloc.pass.cpp:97
+        DerivedFromAllocatorArgT derived;
+        std::tuple<> t(derived, A1<int>());
+    }
----------------
I would like to see tests here for

    std::tuple<int, int> t2(derived, A1<int>(), 42, 42);
    std::tuple<int, int> t3(derived, A1<int>(), std::tuple<int, int>{42, 42});

just to prove that these work for non-empty tuples too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523