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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/tuple:585
+        _BoolConstant<sizeof...(_Tp) >= 1>,
+        _Not<_IsThisTuple<_Up...> >, // extension to allow mis-behaved user constructors
+        is_constructible<_Tp, _Up>...
----------------
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.  ]]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523