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

Fri Feb 12 13:44:50 PST 2021

Quuxplusone 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:
> 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).

================
Comment at: libcxx/include/tuple:917
+        _And<_Dep,
+            _Lazy<_EnableMoveFromPair, _Up1, _Up2>,
+            _Not<_Lazy<_And, // explicit check
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > zoecarver wrote:
> > > > If you're using `_Up1` here, do you need the `_Dep` template type param?
> > > Ok, so that's an excellent question, and I would have expected the answer to be "No". In fact, I initially wrote all those without these `_Lazy` calls whenever some arguments were dependent, but I was getting errors saying (basically): "you're expanding parameter packs of different lengths in the definition of the `_EnableMoveFromPair` alias".
> > > 
> > > I thought that sort of failure within the definition of an alias template would be considered part of the immediate context for SFINAE and it would result in the overload not being selected. But I was getting a hard error instead. I'm not sure whether I'm doing something subtly wrong, or my expectation about substitution failures in alias templates is wrong, or if it's a compiler bug. I decided not to investigate further because this worked, but I can if you request it.
> > I think Zoe is right that you could remove `_Dummy`. I have no opinion on the rest of the simplification you tried. But just this should work, right?:
> > 
> >     template <class _Alloc, class _Up1, class _Up2, _EnableIf<
> >         _And<
> >             _Lazy<_EnableMoveFromPair, _Up1, _Up2>,
> >             _Not<_Lazy<_And, // explicit check
> >                 is_convertible<_Up1, _FirstType<_Tp..., void> >,
> >                 is_convertible<_Up2, _SecondType<_Tp..., void> >
> >             > >
> >         >::value
> >     , int> = 0>
> > 
> > (replacing `_Dummy` with `void` in the places IIUC we don't care what it is)
> > (or maybe you need `SecondType<_Tp..., void, void>`?)
> First, why do we need the `void` types? We've already checked that there are at least two elements, and this should be a short-circuiting operator. 
> 
> Second, I think type alias types should be counted as dependent types as long as they're substituted with template type arguments. So, what Arthur proposed should work. But I can play around with this patch a bit more locally tomorrow and try to get it working. 
> why do we need the `void` types?

Because when `sizeof...(_Tp) == 1`, then `_SecondType<_Tp...>` is ill-formed — which would make the entire type-expression
```
_EnableIf<
    _And<
        _Lazy<_EnableMoveFromPair, _Up1, _Up2>,
        _Not<_Lazy<_And, // explicit check
            is_convertible<_Up1, _FirstType<_Tp..., void> >,
            is_convertible<_Up2, _SecondType<_Tp..., void> >
        > >
    >::value
, int>
```
immediately ill-formed — which would make this constructor template drop out of overload resolution whenever `sizeof...(_Tp) == 1`. Hmm, I guess that's totally OK in this case!

The key thing to watch out for in all this metaprogramming ickiness is that regardless of short-circuiting evaluation of truth //values//, the compiler is still going to be //instantiating// the types that we name in the expression, such as `_FirstType<_Tp..., void>`. I guess that's exactly what `_Lazy` is for, though; and so if `_SecondType<_Tp...>` didn't suffice, the next thing we should try is not `_SecondType<_Tp..., void>` but rather `_Lazy<_SecondType, _Tp...>`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523