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

Thu Feb 11 14:40:30 PST 2021

ldionne added a comment.

Also, I didn't mention it but I didn't add new tests. This is technically not increasing the API surface (in fact decreasing it), and the existing tests seemed to have the right coverage already.

================
Comment at: libcxx/include/tuple:459
 public:
+    // [tuple.cnstr]

----------------
zoecarver wrote:
> The standard has about 100 ways of abbreviating the word "constructor".
Ahah, indeed.

================
Comment at: libcxx/include/tuple:462
+    // tuple() constructors (including allocator_arg_t variants)
+    template <class _Dep = true_type, _EnableIf<
+        _And<_Dep,
----------------
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.

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

================
Comment at: libcxx/include/tuple:489
     _LIBCPP_INLINE_VISIBILITY
-    tuple(_AllocArgT, _Alloc const& __a)
+    tuple(allocator_arg_t, _Alloc const& __a)
       : __base_(allocator_arg_t(), __a,
----------------
zoecarver wrote:
> This constructor could be `_NOEXCEPT_(_And<is_nothrow_default_constructible<_Tp>...>::value)`, right? 
> 
> I think there are a couple of missing noexcepts here, but I think that's better done as a follow-up patch, to keep things separated. I am happy to work on this, after this patch lands. (Also, I won't point it out again, because it's not really relevant to this patch.)
I tried keeping the same `noexcept`s that we had before this patch. None of the `allocator_arg_t` constructors had `noexcept` before the patch, so their don't either after the patch.

I don't really like those constructors :-).

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

================
Comment at: libcxx/include/tuple:917
+        _And<_Dep,
+            _Lazy<_EnableMoveFromPair, _Up1, _Up2>,
+            _Not<_Lazy<_And, // explicit check
----------------
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.

================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/alloc_copy.pass.cpp:57
-#ifdef _LIBCPP_VERSION
     {
         typedef std::tuple<alloc_first, alloc_last> T;
----------------
zoecarver wrote:
> Is this not an extension? If it is an extension, don't we want to keep this behind a condition so that the test suite is more portable?
Actually, I couldn't figure out what about the test case was an extension, so I thought it might have been marked as an extension incorrectly.

================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/alloc_move.pass.cpp:57
-// testing extensions
-#ifdef _LIBCPP_VERSION
     {
----------------
Same here, is this really testing an extension? I can't see what's an extension about it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523