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

More comments later :)



================
Comment at: libcxx/include/tuple:459
 public:
+    // [tuple.cnstr]
 
----------------
The standard has about 100 ways of abbreviating the word "constructor".


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


================
Comment at: libcxx/include/tuple:466
+        >::value
+    ,int> = 0>
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
----------------
Nit: space. 

(Same comment on a few of these ctors.)


================
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,
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523