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

Wed Feb 17 15:18:30 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:
> 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?")
I would like to continue down this rabbit-hole, but I think we're all talking past each other at this point.
- There's the question of pack expansions, like where it's legit to put `is_same<tuple, Up...>` and where not.
- There's the question of eager vs lazy instantiation, like where it's legit to put `(is_convertible<Tp, Up>::value && ...)` and where `_And<is_convertible<Tp, Up>...>` (this is https://bugs.llvm.org/show_bug.cgi?id=23256 as I understand it).
- There's some desire to minimize instantiations, for speed. But there's also an explicit goal to avoid certain completions of `is_convertible<T,U>` because PR23256.
- There's maybe other questions.

(Perhaps `_And<_Not<x>...>` could be replaced by `_NoneOf<x...>`, if our toolbox were expanded to include an implementation of `_NoneOf`.)

I'd like to see either of these next steps:
- @ldionne to just land this thing already, or
- @ldionne to identify a single measurable goal, e.g. "let's figure out how to remove the identifier `_IsThisTuple` while still passing the tests in this patch," which we can then resume rabbit-holing on (but via suggesting actual patches that pass all the tests locally, not just brainstorming untested ideas).

================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/deduct.pass.cpp:16
 // against libstdc++.
-// XFAIL: gcc-5, gcc-6, gcc-7
+// XFAIL: gcc-5, gcc-6, gcc-7, gcc-9, gcc-10

----------------
What about gcc-8?
And does this patch change something that //breaks// gcc-9 and gcc-10, or is it more like "we never tested it and I just now noticed that it's broken"?  Maybe this belongs in a severable commit.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523