[libcxx-commits] [PATCH] D116621: [libc++][P2321R2] Add const overloads to tuple swap, construction and assignment

Thu Mar 24 13:29:03 PDT 2022

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks a lot for the patch! This is a tough one for sure. I thought we would never add new constructors to `tuple`, but it looks like they did it again!

================
Comment at: libcxx/include/tuple:223

+#if _LIBCPP_STD_VER > 20
+template <size_t _Ip, class _Hp, bool _Ep>
----------------
I would *not* guard this with `_LIBCPP_STD_VER` since this is internal. We can guard the top-level `swap` on `std::tuple` only.

================
Comment at: libcxx/include/tuple:225
+template <size_t _Ip, class _Hp, bool _Ep>
+inline _LIBCPP_HIDE_FROM_ABI constexpr void
+swap(const __tuple_leaf<_Ip, _Hp, _Ep>& __x,
----------------
Since this is new code, I would recommend not using `inline` here.

================
Comment at: libcxx/include/tuple:320

+#if _LIBCPP_STD_VER > 20
+    _LIBCPP_HIDE_FROM_ABI constexpr
----------------
Same, I would *not* guard this with `_LIBCPP_STD_VER` since this is internal.

================
Comment at: libcxx/include/tuple:495
+    void swap(const __tuple_impl& __rhs) const noexcept((is_nothrow_swappable_v<const _Tp&> && ...)) {
+        (static_cast<const __tuple_leaf<_Indx, _Tp>&>(*this).swap(static_cast<const __tuple_leaf<_Indx, _Tp>&>(__rhs)),
+         ...);
----------------
Is there a reason for not doing it like we do for non-const types? Let's use the same approach for consistency, whatever that is.

================
Comment at: libcxx/include/tuple:745

     // tuple(const tuple<U...>&) constructors (including allocator_arg_t variants)
+    template <bool _Const, class ..._Up>
----------------
This comment is now incorrect, I would say instead `tuple([const] tuple<U...>&) constructors (including allocator_arg_t variants)` or something like that.

The `&&` comment below is also incorrect now -- please audit to make sure there aren't others too.

================
Comment at: libcxx/include/tuple:754
             _Lazy<_And,
-                _Not<is_convertible<const tuple<_Up>&, _Tp> >...,
-                _Not<is_constructible<_Tp, const tuple<_Up>&> >...
+                  _Not<is_convertible<__maybe_const<_Const, tuple<_Up>>&, _Tp> >...,
+                  _Not<is_constructible<_Tp, __maybe_const<_Const, tuple<_Up>>&> >...
----------------
I don't like making these sorts of comments, but I don't understand why the indentation was changed. It used to be lined up to tabs.

================
Comment at: libcxx/include/tuple:830
     template <class ..._Up>
     struct _EnableMoveFromOtherTuple : _And<
         _Not<is_same<tuple<_Tp...>, tuple<_Up...> > >,
----------------
Don't you need to make changes here too to pass `_Const` and use `__maybe_const<_Const, _Tp>&&` instead? I think this should look like:

```
template <bool _Const, class ..._Up>
struct _EnableMoveFromOtherTuple : _And<
    _Not<is_same<tuple<_Tp...>, tuple<_Up...> > >,
    _Lazy<_Or,
        _BoolConstant<sizeof...(_Tp) != 1>,
        // _Tp and _Up are 1-element packs - the pack expansions look
        // weird to avoid tripping up the type traits in degenerate cases
        _Lazy<_And,
            _Not<is_convertible<__maybe_const<_Const, tuple<_Up>>&&, _Tp> >...,
            _Not<is_constructible<_Tp, __maybe_const<_Const, tuple<_Up>>&& > >...
        >
    >,
    is_constructible<_Tp, __maybe_const<_Const, _Up>&&>...
> { };
```

This should be observable if you have a type `Foo` in your tuple like this:

```
struct Foo {
  Foo(Other const&&);
  Foo(Other&&) = delete;
};
```

If you use `is_constructible<_Tp, _Up>` (where `_Tp=Foo` and `_Up=Other`), we'll prefer the `Foo(Other&&)` constructor and the answer will be "no". If we use my proposed version, it should use `Foo(Other const&&)` instead and be satisfied.

https://godbolt.org/z/b47ffv43P

Please add a test for this in all the constructors that take rvalue-refs.

================
Comment at: libcxx/include/tuple:1015-1017
+    template <class _Alloc,
+              class _U1,
+              class _U2,
----------------
Minor stylistic thing, but I'd do `class _Alloc, class _U1, Class _U2` on the same line for consistency with the rest of the code in this file.

================
Comment at: libcxx/include/tuple:848
     struct _EnableImplicitCopyFromPair : _And<
-        is_constructible<_FirstType<_DependentTp...>, const _Up1&>,
-        is_constructible<_SecondType<_DependentTp...>, const _Up2&>,
----------------
EricWF wrote:
> These bits of SFINAE were written very specifically because tuple is a very weird beast.  
> 
> I'm surprised we don't have a test in the test suite that breaks after changing this, because I thought I added one.
> 
> There are certain SFINAE loops that crop up where checking these properties in the specific order is/was important.
> 
I think what Eric's saying here is that you need to test for `_EnableCopyFromPair` *before* you test the two `is_convertible` conditions.

I think you could witness the difference by using a type that is *not* constructible from `_Up`, but where a conversion from `_Up` triggers a hard error. In that case, the fact that you changed the order of checking will cause you to instantiate the conversion operator before the constructor, and explode. If you had checked for the constructibility first, you'd have gotten a false answer and stopped there, instead of being tripped up by the hard error later on while trying to check whether the constructor should be explicit or not.

Please add tests for this -- we actually see these sorts of funky bad-behaved constructors and conversions in the wild. I remember breaking `nlohmann::json` with a change a couple years ago -- it wasn't fun. I'm not sure whether it ended up being our fault our theirs, but regardless, we want to be bullet proof to avoid breaking widely used libraries, because we look like jokers when that happens :-).

================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_cpp23.pass.cpp:16
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// XFAIL: gcc
+
----------------
Why does it fail on GCC?

================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_cpp23.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please follow the convention used for testing tuple constructors where we have one test file per constructor. It's a real pain, but it's the best way to ensure that we are testing everything. If you want to share code, that's fine, create a local `.h` in that directory.

================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_cpp23.pass.cpp:13
+
+// template <class... UTypes> tuple(const tuple<UTypes...>& u);
+
----------------
Please use `explicit(see-below)`, otherwise it looks like those are always implicit. Applies everywhere.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116621