[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.

What would you think about extending the `_v` type traits to always be available (no matter the standard version)? Then we could remove a lot of the `_And`s and similar types which would mean we would instantiate a lot few types, therefore, having much better compile time. Not a big deal, but it might be nice.



================
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<
----------------
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;
```


================
Comment at: libcxx/include/tuple:462
+    // tuple() constructors (including allocator_arg_t variants)
+    template <class _Dep = true_type, _EnableIf<
+        _And<_Dep,
----------------
ldionne wrote:
> zoecarver wrote:
> > I assume the `_Dep` is just a "dummy type" for the `_EnableIf`? If so, maybe we could keep calling it `_Dummy`, because we use that name in a lot of places in the codebase to indicate that the type doesn't matter, or is just used for a following `_EnableIf`. Right now, when I look at this, I'm wondering if it was changed to add a "dependency" to this constructor (which I don't think is the case). 
> > 
> > Additionally, if you change this to say `class _T2 = _Tp` or `class _Dummy = _Tp` you can omit the `_And`, I think. (Note: here it doesn't really matter because you'd go back to using `__all` but below I think you can eliminate some extra template logic, which is good for both readability and compile time.)
> `_Dep` stands for `_Dependent`. IOW, I'm making the `_And<...>` instantiation dependent on a template argument to avoid it being evaluated eagerly. I can use `_Dummy`, but I went for the shortest thing that made sense since it's used to make `_FirstType<...>` and `_SecondType<...>` dependent below, where having a long name hurts readability. I think `_Dummy` is fine, though.
> 
> > Additionally, if you change this to say class _T2 = _Tp
> 
> I can't do that, because `_Tp` is a parameter pack. Otherwise, I would replace all these occurrences by something notionally equivalent to `<class ..._DependentTp = _Tp>`, and I could get rid of much of the complexity. Unless I've missed something major, that isn't valid C++ right now though.
I suppose `_Dep` makes sense now that I understand what it means. But I still think `_Dummy` is better because that's what we use everywhere else, so thanks for changing it. 

> I can't do that, because _Tp is a parameter pack.

Ah, yes, you're right. For some reason, I thought you could, but it looks like that's not a feature. 


================
Comment at: libcxx/include/tuple:479
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    explicit tuple()
+        _NOEXCEPT_(_And<is_nothrow_default_constructible<_Tp>...>::value)
----------------
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).


================
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:
> 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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523