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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Also, I just realized that we can use fold expressions here (on all supported compilers even in C++03 mode). I bet that would be faster/cleaner than instantiating all the `_And`s.



================
Comment at: libcxx/include/tuple:753
+    template <class ..._Up, _EnableIf<
+        _And<
+            _BoolConstant<sizeof...(_Up) == sizeof...(_Tp)>,
----------------
I think there should be an `(is_constructible_v<T, U&&> && ...)` check.


================
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:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > zoecarver wrote:
> > > > Quuxplusone wrote:
> > > > > 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.
> > > > [[ https://godbolt.org/z/4cKzj9 | This works for me: ]]
> > > > ```
> > > > template<class ...U>
> > > >     static const size_t u_types_constraints =
> > > >         sizeof...(U) == sizeof...(T) &&
> > > >         sizeof...(U) >= 1 &&
> > > >         !is_same<Tuple, U...>::value;
> > > > 
> > > > template<class ...U, class = _EnableIf<u_types_constraints<U...> > >
> > > >     Tuple(U&&...) { }
> > > > ```
> > > > 
> > > > ---
> > > > 
> > > > > 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.
> > > > 
> > > > I also am curious about what this means. I thought it was just to ensure that we picked the right overload (so that this doesn't look like a move constructor when `_Up = tuple`).
> > > Follow up: [[ https://github.com/zoecarver/llvm-project/commit/f9689e12c557dcbcd039ee5bd0b67251fe3ca0d3 | I got this working here ]]. I like this approach much better because we instantiate less than half the types, so it should be much better for compile time.
> > > 
> > > Side note: I spent way too long banging my head against the wall trying to figure out why we can only zip parameter packs in type aliases (and not static vars). It turns out there's an issue for this open: [[ http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1844 | 1844 ]]. Basically, various compilers read "immediate context" differently, so sometimes we get a hard error and sometimes we get a substitution failure. [[ https://godbolt.org/z/erqqGT | Here's a Godbolt to demonstrate the problem.  ]]
> > @zoecarver: Your Godbolt (https://godbolt.org/z/erqqGT ) makes perfect sense to me. Here's a "reduced" (but still icky) version: https://godbolt.org/z/x6c67o
> > If you have a type alias that says "`Yy` is the type you get by zipping these packs," and the packs can't be zipped, well, that's SFINAE-friendly. If you have a static constexpr bool whose //initializer// says "`Xx` is true if zipping these packs gives you foo, or false if zipping these packs gives you bar," and the packs can't be zipped, well, that's //not// SFINAE-friendly (not an "immediate context," if I've got my standardese right).
> > FWIW, I don't understand what the "extension" is here, nor what "mis-behaved user constructors" we're worried about.
> 
> It refers to the test in `libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR23256_constrain_UTypes_ctor.pass.cpp`. That test fails if you remove that line.
> 
> We're on the same page regarding the SFINAE behavior for variable templates vs template aliases, but I don't think it's relevant to the issue directly at hand.
> 
> The issue here is that we want the overload to be enabled when `sizeof...(_Up) > 1`. But when that's the case, `is_same` will cause a SFINAE error because we'll try to instantiate it with too many template arguments. Basically, in order to get rid of `_IsThisTuple`, we'd have to write something like this instead:
> 
> ```
> _And<_BoolConstant<sizeof...(_Up) == 1>, _Lazy<is_same, uncvref_t<_Up>..., tuple> >
> ```
> 
> I don't think that's better, but WDYT?
> But when that's the case, is_same will cause a SFINAE error because we'll try to instantiate it with too many template arguments. 

I think this is the point of confusion. This is also why we //are// able to use a static variable. Because `tuple` is not a parameter pack, so there are never too many template arguments. 

However, in the diff I liked to, Arthur made a really good point, which is that using a static variable might not be faster here if we have to `uncvref_t` all of the `U`s. 

So, I'm happy to ship this version. (Though, there is a small part of my brain saying, "I wonder if it would be faster to use a static variable //and// _IsThisTuple?")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523