[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Tim Shen via Phabricator via cfe-commits]

timshen added a comment.

Created https://reviews.llvm.org/D42510 for discussion. I'm not sure if it's a 
good idea, though.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41977



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Eric Fiselier via cfe-commits]

Yes, it should. That was a silly mistake, and should already have been
fixed in trunk.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Tim Shen via Phabricator via cfe-commits]

timshen added a comment.

`__can_bind_reference()` doesn't return anything when 
__reference_binds_to_temporary doesn't exist. This causes builds break with old 
compilers.

Should it just return true if __reference_binds_to_temporary doesn't exist?


Repository:
  rCXX libc++

https://reviews.llvm.org/D41977



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Eric Fiselier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCXX323380: [libc++] Fix PR20855 -- libc++ incorrectly 
diagnoses illegal reference binding… (authored by EricWF, committed by ).

Repository:
  rCXX libc++

https://reviews.llvm.org/D41977

Files:
  include/tuple
  test/libcxx/utilities/tuple/tuple.tuple/diagnose_reference_binding.fail.cpp
  test/libcxx/utilities/tuple/tuple.tuple/diagnose_reference_binding.pass.cpp
  
test/libcxx/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.fail.cpp
  
test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp

Index: include/tuple
===
--- include/tuple
+++ include/tuple
@@ -173,16 +173,9 @@
 
 template 
 static constexpr bool __can_bind_reference() {
-using _RawTp = typename remove_reference<_Tp>::type;
-using _RawHp = typename remove_reference<_Hp>::type;
-using _CheckLValueArg = integral_constant::value
-||  is_same<_RawTp, reference_wrapper<_RawHp>>::value
-||  is_same<_RawTp, reference_wrapper::type>>::value
->;
-return  !is_reference<_Hp>::value
-|| (is_lvalue_reference<_Hp>::value && _CheckLValueArg::value)
-|| (is_rvalue_reference<_Hp>::value && !is_lvalue_reference<_Tp>::value);
+#if __has_keyword(__reference_binds_to_temporary)
+  return !__reference_binds_to_temporary(_Hp, _Tp);
+#endif
 }
 
 __tuple_leaf& operator=(const __tuple_leaf&);
@@ -224,15 +217,15 @@
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
 explicit __tuple_leaf(_Tp&& __t) _NOEXCEPT_((is_nothrow_constructible<_Hp, _Tp>::value))
 : __value_(_VSTD::forward<_Tp>(__t))
-{static_assert(__can_bind_reference<_Tp>(),
-   "Attempted to construct a reference element in a tuple with an rvalue");}
+{static_assert(__can_bind_reference<_Tp&&>(),
+   "Attempted construction of reference element binds to a temporary whose lifetime has ended");}
 
 template 
 _LIBCPP_INLINE_VISIBILITY
 explicit __tuple_leaf(integral_constant, const _Alloc&, _Tp&& __t)
 : __value_(_VSTD::forward<_Tp>(__t))
-{static_assert(__can_bind_reference<_Tp>(),
-   "Attempted to construct a reference element in a tuple with an rvalue");}
+{static_assert(__can_bind_reference<_Tp&&>(),
+   "Attempted construction of reference element binds to a temporary whose lifetime has ended");}
 
 template 
 _LIBCPP_INLINE_VISIBILITY
Index: test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp
@@ -0,0 +1,136 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+// 
+
+// See llvm.org/PR20855
+
+#include 
+#include 
+#include "test_macros.h"
+
+#if TEST_HAS_BUILTIN_IDENTIFIER(__reference_binds_to_temporary)
+# define ASSERT_REFERENCE_BINDS_TEMPORARY(...) static_assert(__reference_binds_to_temporary(__VA_ARGS__), "")
+# define ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(...) static_assert(!__reference_binds_to_temporary(__VA_ARGS__), "")
+#else
+# define ASSERT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+# define ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+#endif
+
+template 
+struct ConvertsTo {
+  using RawTp = typename std::remove_cv< typename std::remove_reference::type>::type;
+
+  operator Tp() const {
+return static_cast(value);
+  }
+
+  mutable RawTp value;
+};
+
+struct Base {};
+struct Derived : Base {};
+
+
+static_assert(std::is_same::value, "");
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, decltype("abc"));
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, decltype(("abc")));
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, const char*&&);
+
+ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(int&, const ConvertsTo&);
+ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(const int&, ConvertsTo&);
+ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(Base&, Derived&);
+
+
+static_assert(std::is_constructible::value, "");
+static_assert(std::is_constructible>::value, "");
+
+template  struct CannotDeduce {
+ using type = T;
+};
+
+template 
+void 

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Looks good, thanks!

Maybe the assertion message could be clearer about why this is a problem 
though? ("binds to a temporary whose lifetime has ended" maybe?)


https://reviews.llvm.org/D41977



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF updated this revision to Diff 129744.
EricWF added a comment.

- Address @rsmith's comments by removing the fallback implementation of the 
diagnostics.


https://reviews.llvm.org/D41977

Files:
  include/tuple
  test/libcxx/utilities/tuple/tuple.tuple/diagnose_reference_binding.fail.cpp
  test/libcxx/utilities/tuple/tuple.tuple/diagnose_reference_binding.pass.cpp
  
test/libcxx/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.fail.cpp
  
test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp

Index: test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp
===
--- /dev/null
+++ test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp
@@ -0,0 +1,136 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+// 
+
+// See llvm.org/PR20855
+
+#include 
+#include 
+#include "test_macros.h"
+
+#if TEST_HAS_BUILTIN_IDENTIFIER(__reference_binds_to_temporary)
+# define ASSERT_REFERENCE_BINDS_TEMPORARY(...) static_assert(__reference_binds_to_temporary(__VA_ARGS__), "")
+# define ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(...) static_assert(!__reference_binds_to_temporary(__VA_ARGS__), "")
+#else
+# define ASSERT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+# define ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+#endif
+
+template 
+struct ConvertsTo {
+  using RawTp = typename std::remove_cv< typename std::remove_reference::type>::type;
+
+  operator Tp() const {
+return static_cast(value);
+  }
+
+  mutable RawTp value;
+};
+
+struct Base {};
+struct Derived : Base {};
+
+
+static_assert(std::is_same::value, "");
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, decltype("abc"));
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, decltype(("abc")));
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, const char*&&);
+
+ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(int&, const ConvertsTo&);
+ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(const int&, ConvertsTo&);
+ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(Base&, Derived&);
+
+
+static_assert(std::is_constructible::value, "");
+static_assert(std::is_constructible>::value, "");
+
+template  struct CannotDeduce {
+ using type = T;
+};
+
+template 
+void F(typename CannotDeduce::type const&) {}
+
+void compile_tests() {
+  {
+F(std::make_tuple(42, 42));
+  }
+  {
+F(std::make_tuple(42, 42));
+std::tuple t(std::make_tuple(42, 42));
+  }
+  {
+auto fn = ;
+fn(std::tuple(42, std::string("a")));
+fn(std::make_tuple(42, std::string("a")));
+  }
+  {
+Derived d;
+std::tuple t(d, d);
+  }
+  {
+ConvertsTo ct;
+std::tuple t(42, ct);
+  }
+}
+
+void allocator_tests() {
+std::allocator alloc;
+int x = 42;
+{
+std::tuple t(std::ref(x));
+assert(::get<0>(t) == );
+std::tuple t1(std::allocator_arg, alloc, std::ref(x));
+assert(::get<0>(t1) == );
+}
+{
+auto r = std::ref(x);
+auto const& cr = r;
+std::tuple t(r);
+assert(::get<0>(t) == );
+std::tuple t1(cr);
+assert(::get<0>(t1) == );
+std::tuple t2(std::allocator_arg, alloc, r);
+assert(::get<0>(t2) == );
+std::tuple t3(std::allocator_arg, alloc, cr);
+assert(::get<0>(t3) == );
+}
+{
+std::tuple t(std::ref(x));
+assert(::get<0>(t) == );
+std::tuple t2(std::cref(x));
+assert(::get<0>(t2) == );
+std::tuple t3(std::allocator_arg, alloc, std::ref(x));
+assert(::get<0>(t3) == );
+std::tuple t4(std::allocator_arg, alloc, std::cref(x));
+assert(::get<0>(t4) == );
+}
+{
+auto r = std::ref(x);
+auto cr = std::cref(x);
+std::tuple t(r);
+assert(::get<0>(t) == );
+std::tuple t2(cr);
+assert(::get<0>(t2) == );
+std::tuple t3(std::allocator_arg, alloc, r);
+assert(::get<0>(t3) == );
+std::tuple t4(std::allocator_arg, alloc, cr);
+assert(::get<0>(t4) == );
+}
+}
+
+
+int main() {
+  compile_tests();
+  allocator_tests();
+}
Index: test/libcxx/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.fail.cpp

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

In https://reviews.llvm.org/D41977#975403, @rsmith wrote:

> This will still diagnose valid and reasonable programs, such as:
>
>   struct ConvertToRef { operator int&(); };
>   std::tuple t = {ConvertToRef()};
>
>
> ... on compilers that don't provide the trait. You could maybe try to work 
> around that by checking to see if the type has a member `.operator int&()`. 
> But perhaps it's better to remove the non-conforming check entirely, at least 
> in the case where you can't reasonably get it right.


I agree with both your comments. I'll remove the non-conforming check.


https://reviews.llvm.org/D41977



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This will still diagnose valid and reasonable programs, such as:

  struct ConvertToRef { operator int&(); };
  std::tuple t = {ConvertToRef()};

... on compilers that don't provide the trait. You could maybe try to work 
around that by checking to see if the type has a member `.operator int&()`. But 
perhaps it's better to remove the non-conforming check entirely, at least in 
the case where you can't reasonably get it right.


https://reviews.llvm.org/D41977



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/tuple:185-186
+// Allow "int&&" to bind to 'int const&'
+||  (is_rvalue_reference<_Tp>::value && is_const<_RawHp>::value &&
+is_same<_RawHp, const _RawTp>::value)
 >;

It would be reasonable to consider `is_base_of` here too.



Comment at: include/tuple:190
 || (is_lvalue_reference<_Hp>::value && _CheckLValueArg::value)
 || (is_rvalue_reference<_Hp>::value && 
!is_lvalue_reference<_Tp>::value);
+#else

This line looks wrong to me. This disallows an rvalue reference tuple member 
from binding to an rvalue reference argument, no? Can you
```
return !is_reference<_Hp>::value || (is_reference<_Tp>::value && 
is_convertible<_RawTp*, _RawHp*>::value) || (reference wrapper special case);
```
instead? That should at least only reject valid code in cases where `_RawTp` is 
a class type that converts to a reference type.


https://reviews.llvm.org/D41977



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41977: [libc++] Fix PR20855 -- libc++ incorrectly diagnoses illegal reference binding in std::tuple.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF created this revision.
EricWF added reviewers: rsmith, mclow.lists.

See https://bugs.llvm.org/show_bug.cgi?id=20855

Libc++ goes out of it's way to diagnose `std::tuple` constructions which are UB 
due to lifetime bugs caused by reference creation. For example:

  // The 'const std::string&' is created *inside* the tuple constructor, and 
its lifetime is over before the end of the constructor call.
  std::tuple t(std::make_tuple(42, "abc"));

However, we are over-aggressive and we incorrectly diagnose cases such as:

  void foo(std::tuple const&);
  foo(std::make_tuple(42, 42));

This patch fixes the incorrectly diagnosed cases, as well as converting the 
diagnostic to use the newly added Clang trait `__reference_binds_to_temporary`. 
The new trait allows us to diagnose cases we previously couldn't such as:

  std::tuple t(42, "abc");


https://reviews.llvm.org/D41977

Files:
  include/tuple
  
test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.fail.cpp
  
test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp

Index: test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp
===
--- /dev/null
+++ test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.pass.cpp
@@ -0,0 +1,47 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+// 
+
+// See llvm.org/PR20855
+
+#include 
+#include 
+#include "test_macros.h"
+
+#if TEST_HAS_BUILTIN_IDENTIFIER(__reference_binds_to_temporary)
+# define ASSERT_REFERENCE_BINDS_TEMPORARY(...) static_assert(__reference_binds_to_temporary(__VA_ARGS__), "")
+# define ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(...) static_assert(!__reference_binds_to_temporary(__VA_ARGS__), "")
+#else
+# define ASSERT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+# define ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+#endif
+
+static_assert(std::is_same::value, "");
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, decltype("abc"));
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, decltype(("abc")));
+ASSERT_REFERENCE_BINDS_TEMPORARY(std::string const&, const char*&&);
+
+template >
+void F(Tup const&) {}
+
+int main() {
+  {
+F(std::make_tuple(42, 42));
+std::tuple t(std::make_tuple(42, 42));
+  }
+  {
+auto fn = ;
+fn(std::tuple(42, std::string("a")));
+fn(std::make_tuple(42, std::string("a")));
+  }
+}
Index: test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.fail.cpp
===
--- /dev/null
+++ test/std/utilities/tuple/tuple.tuple/tuple.cnstr/PR20855_tuple_ref_binding_diagnostics.fail.cpp
@@ -0,0 +1,54 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03
+
+// 
+
+// See llvm.org/PR20855
+
+#ifdef __clang__
+#pragma clang diagnostic ignored "-Wdangling-field"
+#endif
+
+#include 
+#include 
+#include "test_macros.h"
+
+
+#if !TEST_HAS_BUILTIN_IDENTIFIER(__reference_binds_to_temporary)
+#else
+# define ASSERT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+# define ASSERT_NOT_REFERENCE_BINDS_TEMPORARY(...) static_assert(true, "")
+#endif
+
+template  struct CannotDeduce {
+ using type = T;
+};
+
+template 
+void F(typename CannotDeduce::type const&) {}
+
+
+int main() {
+  // expected-error@tuple:* 1 {{"Attempted to construct a reference element in a tuple with an rvalue"}}
+  {
+F(std::make_tuple(1, "abc")); // expected-note 1 {{requested here}}
+  }
+#if TEST_HAS_BUILTIN_IDENTIFIER(__reference_binds_to_temporary)
+  // expected-error@tuple:* 2 {{"Attempted to construct a reference element in a tuple with an rvalue"}}
+  {
+std::tuple t(1, "a"); // expected-note 1 {{requested here}}
+  }
+  {
+F(std::tuple(1, "abc")); // expected-note 1 {{requested here}}
+  }
+#endif
+}
Index: include/tuple