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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Feb 15 11:01:55 PST 2021


ldionne added a comment.

I addressed all comments with a direct suggestion. For other comments on using various patterns for doing SFINAE, I actually reworked everything to make GCC happy.

You'll notice that we now never call a metafunction with the incorrect number of arguments. GCC does not seem to handle SFINAE failures for mismatched parameter pack lengths correctly, so I had to use this approach.

I still need to investigate a test failure in C++11 and C++14 mode - the behavior after this patch matches the libstdc++ behavior, so I think the test was wrong, but I need to understand it better.



================
Comment at: libcxx/include/tuple:479
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    explicit tuple()
+        _NOEXCEPT_(_And<is_nothrow_default_constructible<_Tp>...>::value)
----------------
zoecarver wrote:
> ldionne wrote:
> > zoecarver wrote:
> > > When were these conditionally explicit overloads added? I think C++17. 
> > > 
> > > This is a bit annoying because in C++20 we get the `explicit(bool)` operator which would make this implementation about 50% smaller. 
> > > 
> > > It looks like that was added to clang in V10. I wonder if we could get away with a macro to do this. There would be a small overlap of users on the V6-V10 clang version using C++17 who might be affected. 
> > > When were these conditionally explicit overloads added? I think C++17.
> > 
> > Hmm, I'm not 100% sure, but that sounds about right. Perhaps we've been supporting those in older Standards as an extension. While I generally dislike extensions, I think that not enabling those implicit/explicit constructors pre-C++17 would be a pretty big cost in complexity for little benefit, so I'd rather implement them even pre-C++17. Open to discussion, of course.
> > 
> > > This is a bit annoying because in C++20 we get the explicit(bool) operator
> > 
> > Yes, I agree, those are a huge pain to write without `explicit(bool)`, but I don't think there's a way around that TBH. To be fair, there is quite a bit of code duplication, but it's simple code. I was also planning on simplifying the calls to the `__base_` constructor in a follow-up patch to reduce the magnitude of the duplication. Let me know if you have a blocking comment or if you want me to take an action here.
> > While I generally dislike extensions, I think that not enabling those implicit/explicit constructors pre-C++17 would be a pretty big cost in complexity for little benefit, so I'd rather implement them even pre-C++17.
> 
> I think if we removed the extension, we'd actually reduce the complexity significantly. Here's what I had in mind:
> ```
> #if STD > 14 && CLANG >=10
> #define _EXPLICIT_COND(...) explicit(...)
> #else
> #define _EXPLICIT_COND
> #endif
> ```
> Then we could just annotate all these overloads with `_EXPLICIT_COND(...)`. The problem is we'd be non-conforming for those using C++17 on clang V9 or lower, and this would potentially break people's code if they were on C++11 or 14. WDYT? Could we get away with it? 😉
> 
> This is a non-blocking comment. I just thought it might be a way to reduce a bunch of code (but it looks it probably won't work for now, but maybe we can come back to it in a few years).
Does Clang trunk provide `explicit(bool)` as an extension in C++ < 20 modes? If so, then I suggest we come back once we drop support for older Clangs and remove the duplication.

Otherwise, if Clang doesn't support `explicit(bool)` in pre-C++20 mode, we will never be able to remove this duplication because we still have to implement the explicit/implicit constructors for tuple pre-C++20 (it was applied as a LWG issue IIRC).


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


================
Comment at: libcxx/include/tuple:917
+        _And<_Dep,
+            _Lazy<_EnableMoveFromPair, _Up1, _Up2>,
+            _Not<_Lazy<_And, // explicit check
----------------
Quuxplusone wrote:
> 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...>`.
> the next thing we should try is not `_SecondType<_Tp..., void>` but rather `_Lazy<_SecondType, _Tp...>`.

That doesn't work, because `_SecondType` is an alias that expands to the type itself. So `_Lazy` would end up inheriting from that type, which isn't what we want.


================
Comment at: libcxx/include/tuple:794
+    // tuple(const pair<U1, U2>&) constructors (including allocator_arg_t variants)
+    template <class _Up1, class _Up2, class _Dummy = void>
+    using _EnableCopyFromPair = _And<
----------------
zoecarver wrote:
> Wait... why do we have `_Dummy` types //here//? Why can't this just be:
> ```
> template <class U1, class U2>
> static const size_t __enable_copy_from_pair = sizeof...(_Tp) == 2 && 
>     is_constructible<_FirstType <_Tp...>, const U1&>::value &&
>     is_constructible<_SecondType<_Tp...>, const U2&>::value;
> ```
Because when `sizeof...(_Tp) < 2`, the "expression" `_FirstType<_Tp...>` is a hard error. That "expression" is "evaluated" when the `tuple` template is instantiated, not when we instantiate the template alias.


================
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()
----------------
Quuxplusone wrote:
> Nit: `when there are fewer initializers than elements` or `fewer constructor arguments than tuple elements`.
Thanks, much better indeed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523



More information about the libcxx-commits mailing list