[libcxx-commits] [libcxx] 17f2d1c - [libc++] Fix QoI bug with construction of std::tuple involving std::any

[Louis Dionne via libcxx-commits]

Author: Louis Dionne
Date: 2021-05-04T16:42:36-04:00
New Revision: 17f2d1cb9b93d336d4187cd14307bef1ab535808

URL: https://github.com/llvm/llvm-project/commit/17f2d1cb9b93d336d4187cd14307bef1ab535808
DIFF: https://github.com/llvm/llvm-project/commit/17f2d1cb9b93d336d4187cd14307bef1ab535808.diff

LOG: [libc++] Fix QoI bug with construction of std::tuple involving std::any

In std::tuple, we should try to avoid calling std::is_copy_constructible
whenever we can to avoid surprising interactions with (I believe) compiler
builtins. This bug was reported in https://reviews.llvm.org/D96523#2730953.

The issue was that when tuple<_Up...> was the same as tuple<_Tp...>, we
would short-circuit the _Or (because sizeof...(_Tp) != 1) and go evaluate
the following `is_constructible<_Tp, const _Up&>...`. That shouldn't
actually be a problem, but see the analysis in https://reviews.llvm.org/D101770#2736470
for why it is with Clang and GCC.

Instead, after this patch, we check whether the constructed-from tuple
is the same as the current tuple regardless of the number of elements,
since we should always prefer the normal copy constructor in that case
anyway.

Differential Revision: https://reviews.llvm.org/D101770

Added: 
    libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/cnstr_with_any.compile.pass.cpp

Modified: 
    libcxx/include/tuple

Removed: 
    


################################################################################
diff  --git a/libcxx/include/tuple b/libcxx/include/tuple
index fa0b6a03cc74..0a131f302ae3 100644
--- a/libcxx/include/tuple
+++ b/libcxx/include/tuple
@@ -675,12 +675,12 @@ public:
     // tuple(const tuple<U...>&) constructors (including allocator_arg_t variants)
     template <class ..._Up>
     struct _EnableCopyFromOtherTuple : _And<
-        _Or<
+        _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_same<_Tp, _Up> >...,
                 _Not<is_convertible<const tuple<_Up>&, _Tp> >...,
                 _Not<is_constructible<_Tp, const tuple<_Up>&> >...
             >
@@ -741,12 +741,12 @@ public:
     // tuple(tuple<U...>&&) constructors (including allocator_arg_t variants)
     template <class ..._Up>
     struct _EnableMoveFromOtherTuple : _And<
-        _Or<
+        _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_same<_Tp, _Up> >...,
                 _Not<is_convertible<tuple<_Up>, _Tp> >...,
                 _Not<is_constructible<_Tp, tuple<_Up> > >...
             >

diff  --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/cnstr_with_any.compile.pass.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/cnstr_with_any.compile.pass.cpp
new file mode 100644
index 000000000000..5c6ca1c7996f
--- /dev/null
+++ b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/cnstr_with_any.compile.pass.cpp
@@ -0,0 +1,76 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// This test makes sure that we can copy/move a std::tuple containing a type
+// that checks for copy constructibility itself, like std::any.
+//
+// Problem showcased in https://reviews.llvm.org/D96523#2730953.
+
+#include <any>
+#include <tuple>
+#include <type_traits>
+#include <utility>
+
+#include "test_macros.h"
+
+template <class ...Pred>
+struct And : std::true_type { };
+
+template <class P1, class ...Pn>
+struct And<P1, Pn...>
+  : std::conditional<P1::value, And<Pn...>, std::false_type>::type
+{ };
+
+struct any {
+  any();
+  any(any const&) = default;
+
+  template <class ValueType,
+            class Decayed = typename std::decay<ValueType>::type,
+            class = typename std::enable_if<
+              !std::is_same<Decayed, any>::value &&
+              std::is_copy_constructible<Decayed>::value
+            >::type>
+  any(ValueType&&);
+};
+
+struct A {
+  A();
+  A(any);
+};
+
+#if TEST_STD_VER > 14
+struct B {
+  B();
+  B(std::any);
+};
+#endif
+
+void f() {
+  {
+    std::tuple<A, int> x;
+    std::tuple<A, int> y = x; (void)y;
+  }
+  {
+    std::tuple<A, int> x;
+    std::tuple<A, int> y = std::move(x); (void)y;
+  }
+
+#if TEST_STD_VER > 14
+  {
+    std::tuple<B, int> x;
+    std::tuple<B, int> y = x; (void)y;
+  }
+  {
+    std::tuple<B, int> x;
+    std::tuple<B, int> y = std::move(x); (void)y;
+  }
+#endif
+}