[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG74f93bc373d0: [Sema] Fix deleted function problem in 
implicitly movable test (authored by nullptr.cpp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
  clang/test/SemaCXX/warn-return-std-move.cpp

Index: clang/test/SemaCXX/warn-return-std-move.cpp
===
--- clang/test/SemaCXX/warn-return-std-move.cpp
+++ clang/test/SemaCXX/warn-return-std-move.cpp
@@ -324,11 +324,29 @@
 // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:12}:"std::move(d)"
 }
 
-void ok_throw1() { Derived d; throw d; }
+void ok_throw1() {
+  Derived d;
+  throw d;
+}
 void ok_throw2(Derived d) { throw d; }
-void ok_throw3(Derived& d) { throw d; }
+void ok_throw3(Derived ) { throw d; }
 void ok_throw4(Derived d) { throw std::move(d); }
-void ok_throw5(Derived& d) { throw std::move(d); }
-void ok_throw6(Derived& d) { throw static_cast(d); }
+void ok_throw5(Derived ) { throw std::move(d); }
+void ok_throw6(Derived ) { throw static_cast(d); }
 void ok_throw7(TriviallyCopyable d) { throw d; }
 void ok_throw8(OnlyCopyable d) { throw d; }
+
+namespace test_delete {
+struct Base {
+  Base();
+  Base(Base &&) = delete;
+  Base(Base const &);
+};
+
+struct Derived : public Base {};
+
+Base test_ok() {
+  Derived d;
+  return d;
+}
+} // namespace test_delete
Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3118,11 +3118,16 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema , const InitializedEntity ,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
   QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
-  ExprResult ) {
+  bool IsDiagnosticsCheck, ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
 CK_NoOp, Value, VK_XValue, FPOptionsOverride());
 
@@ -3133,8 +3138,11 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq &&
+  (IsDiagnosticsCheck || Seq.getFailedOverloadResult() != OR_Deleted)) {
+return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3177,6 +3185,7 @@
   

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[dmajor via Phabricator via cfe-commits]

dmajor added a comment.

I tried the updated patch against our build and it was successful. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:4020
  bool IsListInit = false,
  bool IsInitListCopy = false) {
   assert(((!IsListInit && !IsInitListCopy) ||

Two comments you could act on if you want, or just ignore if they seem too 
scope-creepy:

(1) This function is //crazy// long and complicated. It would be great to break 
it down into named steps or subtasks.

(2) I'm still concerned about the repetition of `if (Result == OR_Deleted but 
Kind == IK_Copy) keep going`. IIUC, the intent is that this function should 
return the `InitializationSequence` that is found by overload resolution... but 
sometimes we can tell that overload resolution is going to find an 
inappropriate sequence (e.g. an explicit conversion, or a sequence that depends 
on a deleted function, or maybe some other situations) and in that case we want 
to short-circuit, because the caller is going to treat "No unique sequence" in 
the same way as "The unique sequence was inappropriate".  **Would it be better 
to** have the caller pass in a new function parameter, `bool 
ShortCircuitUnusableSequences`, which is usually `true` but can be explicitly 
set to `false` in the three cases (`return`, `co_return`, `throw`) where we 
currently want to treat "No unique sequence" differently from "The unique 
sequence was inappropriate"?

Then the repeated check would be `if (Result == OR_Deleted && 
ShortCircuitUnusableSequences) return;` and it would be a little clearer what's 
going on.

(Sidenote: Default function arguments, //especially// trailing boolean ones, 
[are the 
devil](https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil/#the-boolean-parameter-tarpit).)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp updated this revision to Diff 314509.
nullptr.cpp added a comment.

Fix use-of-uninitialized-value and `-Wreturn-std-move` with delete function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
  clang/test/SemaCXX/warn-return-std-move.cpp

Index: clang/test/SemaCXX/warn-return-std-move.cpp
===
--- clang/test/SemaCXX/warn-return-std-move.cpp
+++ clang/test/SemaCXX/warn-return-std-move.cpp
@@ -324,11 +324,29 @@
 // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:12}:"std::move(d)"
 }
 
-void ok_throw1() { Derived d; throw d; }
+void ok_throw1() {
+  Derived d;
+  throw d;
+}
 void ok_throw2(Derived d) { throw d; }
-void ok_throw3(Derived& d) { throw d; }
+void ok_throw3(Derived ) { throw d; }
 void ok_throw4(Derived d) { throw std::move(d); }
-void ok_throw5(Derived& d) { throw std::move(d); }
-void ok_throw6(Derived& d) { throw static_cast(d); }
+void ok_throw5(Derived ) { throw std::move(d); }
+void ok_throw6(Derived ) { throw static_cast(d); }
 void ok_throw7(TriviallyCopyable d) { throw d; }
 void ok_throw8(OnlyCopyable d) { throw d; }
+
+namespace test_delete {
+struct Base {
+  Base();
+  Base(Base &&) = delete;
+  Base(Base const &);
+};
+
+struct Derived : public Base {};
+
+Base test_ok() {
+  Derived d;
+  return d;
+}
+} // namespace test_delete
Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3118,11 +3118,16 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema , const InitializedEntity ,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
   QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
-  ExprResult ) {
+  bool IsDiagnosticsCheck, ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
 CK_NoOp, Value, VK_XValue, FPOptionsOverride());
 
@@ -3133,8 +3138,11 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq &&
+  (IsDiagnosticsCheck || Seq.getFailedOverloadResult() != OR_Deleted)) {
+return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3177,6 +3185,7 @@
   }
 }
 
+

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D92936#2477764 , @dmajor wrote:

> Before the revert, our bots hit the following issue where we only error out 
> when `-Wall` is given, so there's definitely something strange going on.

https://godbolt.org/z/P1dv9f
Yeah, I see what's happening here. `-Wall` turns on `-Wreturn-std-move`, which 
in C++17 mode does a preliminary overload resolution "as if by std::move" just 
to see what would happen. This preliminary overload resolution finds a deleted 
function, and apparently this is a hard error — we're not doing whatever dance 
Clang requires in order to suppress/defer diagnostics during "speculative" 
compilation. (The difficulty of speculatively compiling things is one of my pet 
peeves with Clang.)

Here's an example using Clang trunk to produce a hard error from the guts of 
`-Wreturn-std-move` (left-hand pane) but no error if you don't enable that 
warning (right-hand pane).
https://godbolt.org/z/e6zPsb
However, this might be Clang-trunk-including-Yang's-patch, I'm not sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[dmajor via Phabricator via cfe-commits]

dmajor added a comment.

Before the revert, our bots hit the following issue where we only error out 
when `-Wall` is given, so there's definitely something strange going on. Also 
happens with @Quuxplusone's suggested change applied.

  $ cat test.cpp
  template < class > class RefPtr {
  public:
~RefPtr();
template < typename d > RefPtr(d);
operator int() &;
operator int() && = delete;
  };
  class X;
  bool e() {
RefPtr< X > frame(0);
return frame;
  }
  
  $ clang -cc1 -std=c++17 test.cpp
  $ clang -cc1 -std=c++17 test.cpp -Wall
  test.cpp:11:10: error: conversion function from 'RefPtr' to 'bool' invokes 
a deleted function
return frame;
   ^
  test.cpp:6:3: note: 'operator int' has been explicitly marked deleted here
operator int() && = delete;
^
  1 error generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D92936#2476356 , @vitalybuka wrote:

> Something is not initialized
> http://lab.llvm.org:8011/#/builders/74/builds/1834/steps/9/logs/stdio

Confirmed; @nullptr.cpp what do you want to do about this? I hypothesize that 
maybe you're not allowed to look at `Seq.getFailedOverloadResult()` (nor 
`Seq.getFailedCandidateSet()`) unless `Seq.getFailureKind()` is one of 
`FK_ConstructorOverloadFailed`, `FK_UserConversionOverloadFailed`, 
`FK_ReferenceInitOverloadFailed`, or `FK_ListConstructorOverloadFailed`.  The 
ctor `InitializationSequence::InitializationSequence` member-initializes 
`FailedCandidateSet` but does not member-initialize `FailedOverloadResult`. 
Perhaps the appropriate fix would be

  --- a/clang/lib/Sema/SemaInit.cpp
  +++ b/clang/lib/Sema/SemaInit.cpp
  @@ -5595,7 +5595,8 @@ InitializationSequence::InitializationSequence(Sema ,
  MultiExprArg Args,
  bool TopLevelOfInitList,
  bool 
TreatUnavailableAsInvalid)
  -: FailedCandidateSet(Kind.getLocation(), 
OverloadCandidateSet::CSK_Normal) {
  +: FailedOverloadResult(OR_Success),
  +  FailedCandidateSet(Kind.getLocation(), 
OverloadCandidateSet::CSK_Normal) {
 InitializeFrom(S, Entity, Kind, Args, TopLevelOfInitList,
TreatUnavailableAsInvalid);

I've tested this locally and it doesn't cause any new tests to fail (whew!), 
but I haven't rebuilt everything with MSAN to see if this satisfies Vitaly's 
buildbot (and in fact since I'm on OSX I don't think I //can// rebuild with 
MSAN because OSX doesn't support MSAN).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

Something is not initialized 
tihttp://lab.llvm.org:8011/#/builders/74/builds/1834/steps/9/logs/stdio


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Thanks, and checking other compilers with godbolt is a good idea (I had tried 
gcc locally but it was likely too old)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D92936#2476091 , @aeubanks wrote:

> This broke [ https://godbolt.org/z/9EG7ev ]
> is this intentional?

Yes, intentional. This brings Clang's C++14 conformance into line with 
GCC/EDG/MSVC, who all already (correctly) reject that code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

This broke

  struct A {
A();
A(A&&)=delete;
   private:
A(const A&);
   friend class B;
  };
  
  struct B {
A foo() {
  A a;
  return a;
}
  };

with

  /tmp/a.cc:12:12: error: call to deleted constructor of 'A'
  return a;
 ^
  /tmp/a.cc:3:3: note: 'A' has been explicitly marked deleted here
A(A&&)=delete;
^
  1 error generated.

is this intentional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG89b0972aa2f5: [Sema] Fix deleted function problem in 
implicitly movable test (authored by nullptr.cpp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3118,11 +3118,14 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema& S,
-  const InitializedEntity ,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
-  QualType ResultType,
-  Expr *,
+  QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
   ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
@@ -3135,8 +3138,10 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq && Seq.getFailedOverloadResult() != OR_Deleted) {
+return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3179,6 +3184,7 @@
   }
 }
 
+NeedSecondOverloadResolution = false;
 // Promote "AsRvalue" to the heap, since we now need this
 // expression node to persist.
 Value =
@@ -3189,6 +3195,8 @@
 // using the constructor we found.
 Res = Seq.Perform(S, Entity, Kind, Value);
   }
+
+  return NeedSecondOverloadResolution;
 }
 
 /// Perform the initialization of a potentially-movable value, which
@@ -3213,6 +3221,7 @@
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
+  bool NeedSecondOverloadResolution = true;
 
   if (AllowNRVO) {
 bool AffectedByCWG1579 = false;
@@ -3229,11 +3238,11 @@
 }
 
 if (NRVOCandidate) {
-  TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-true, Res);
+  NeedSecondOverloadResolution = TryMoveInitialization(
+  *this, Entity, NRVOCandidate, ResultType, Value, true, Res);
 }
 
-if (!Res.isInvalid() && AffectedByCWG1579) {
+if (!NeedSecondOverloadResolution && AffectedByCWG1579) {
   QualType QT = NRVOCandidate->getType();
   if (QT.getNonReferenceType()
   

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp updated this revision to Diff 314208.
nullptr.cpp added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3118,11 +3118,14 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema& S,
-  const InitializedEntity ,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
-  QualType ResultType,
-  Expr *,
+  QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
   ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
@@ -3135,8 +3138,10 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq && Seq.getFailedOverloadResult() != OR_Deleted) {
+return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3179,6 +3184,7 @@
   }
 }
 
+NeedSecondOverloadResolution = false;
 // Promote "AsRvalue" to the heap, since we now need this
 // expression node to persist.
 Value =
@@ -3189,6 +3195,8 @@
 // using the constructor we found.
 Res = Seq.Perform(S, Entity, Kind, Value);
   }
+
+  return NeedSecondOverloadResolution;
 }
 
 /// Perform the initialization of a potentially-movable value, which
@@ -3213,6 +3221,7 @@
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
+  bool NeedSecondOverloadResolution = true;
 
   if (AllowNRVO) {
 bool AffectedByCWG1579 = false;
@@ -3229,11 +3238,11 @@
 }
 
 if (NRVOCandidate) {
-  TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-true, Res);
+  NeedSecondOverloadResolution = TryMoveInitialization(
+  *this, Entity, NRVOCandidate, ResultType, Value, true, Res);
 }
 
-if (!Res.isInvalid() && AffectedByCWG1579) {
+if (!NeedSecondOverloadResolution && AffectedByCWG1579) {
   QualType QT = NRVOCandidate->getType();
   if (QT.getNonReferenceType()
  .getUnqualifiedType()
@@ -3256,7 +3265,7 @@
 Diag(Value->getExprLoc(), 

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp updated this revision to Diff 313828.
nullptr.cpp added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3118,11 +3118,14 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema& S,
-  const InitializedEntity ,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
-  QualType ResultType,
-  Expr *,
+  QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
   ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
@@ -3135,8 +3138,10 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq && Seq.getFailedOverloadResult() != OR_Deleted) {
+return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3179,6 +3184,7 @@
   }
 }
 
+NeedSecondOverloadResolution = false;
 // Promote "AsRvalue" to the heap, since we now need this
 // expression node to persist.
 Value =
@@ -3189,6 +3195,8 @@
 // using the constructor we found.
 Res = Seq.Perform(S, Entity, Kind, Value);
   }
+
+  return NeedSecondOverloadResolution;
 }
 
 /// Perform the initialization of a potentially-movable value, which
@@ -3213,6 +3221,7 @@
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
+  bool NeedSecondOverloadResolution = true;
 
   if (AllowNRVO) {
 bool AffectedByCWG1579 = false;
@@ -3229,11 +3238,11 @@
 }
 
 if (NRVOCandidate) {
-  TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-true, Res);
+  NeedSecondOverloadResolution = TryMoveInitialization(
+  *this, Entity, NRVOCandidate, ResultType, Value, true, Res);
 }
 
-if (!Res.isInvalid() && AffectedByCWG1579) {
+if (!NeedSecondOverloadResolution && AffectedByCWG1579) {
   QualType QT = NRVOCandidate->getType();
   if (QT.getNonReferenceType()
  .getUnqualifiedType()
@@ -3256,7 +3265,7 @@
 Diag(Value->getExprLoc(), 

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur O'Dwyer via Phabricator via cfe-commits]

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

I don't fully understand the new control flow, but at least the new 
//behavior// (after applying this patch) looks like an improvement to me.
I recommend rebasing on top-of-tree, mainly so that the buildbots will run 
again and presumably be greener this time.

I still doubt that I have the authority to "accept" this patch, and am hoping 
to see someone like @zygoloid weigh in, even if it's only to say "meh." I 
//am// willing and able to //physically// land this patch, if someone says it's 
appropriate for me to do so.




Comment at: clang/lib/Sema/SemaInit.cpp:4181
+  // For deleted functions in other contexts, there is no need to get the
+  // initialization sequence.
+  if (Result == OR_Deleted && Kind.getKind() != InitializationKind::IK_Copy)

IIUC, this comment is explaining the motivation for repeating `if (Result == 
OR_Deleted) // don't return quite yet` twice in the code above — line 4123 and 
line 4146. It might be better to move the comment higher, then.



Comment at: clang/lib/Sema/SemaInit.cpp:5285
+// need the initialization sequence to decide whether to perform the second
+// overload resolution.
+if (!(Result == OR_Deleted &&

Checking my understanding: If the first resolution selects a deleted function 
which is a constructor whose first parameter is an rvalue reference to T, then 
we don't perform the second resolution. If the first resolution selects a 
deleted function which is not a constructor, or whose parameter is of the wrong 
type, then (in C++11 through C++17 but not in C++20) we //do// perform the 
second resolution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp added a comment.

Note: The failed test has nothing to do with this patch.




Comment at: clang/lib/Sema/SemaInit.cpp:4146
   CXXConstructorDecl *CtorDecl = cast(Best->Function);
-  if (Kind.getKind() == InitializationKind::IK_Default &&
-  Entity.getType().isConstQualified()) {
-if (!CtorDecl->getParent()->allowConstDefaultInit()) {
-  if (!maybeRecoverWithZeroInitialization(S, Sequence, Entity))
-Sequence.SetFailed(InitializationSequence::FK_DefaultInitOfConst);
+  if (Result != OR_Deleted) { // TODO: Support for more than one failure.
+// C++11 [dcl.init]p6:

I'll send another patch to make Clang report both deleted function error and 
the following error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp updated this revision to Diff 311158.
nullptr.cpp added a comment.

Fix format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3106,11 +3106,14 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema& S,
-  const InitializedEntity ,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
-  QualType ResultType,
-  Expr *,
+  QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
   ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
@@ -3123,8 +3126,10 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq && Seq.getFailedOverloadResult() != OR_Deleted) {
+return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3167,6 +3172,7 @@
   }
 }
 
+NeedSecondOverloadResolution = false;
 // Promote "AsRvalue" to the heap, since we now need this
 // expression node to persist.
 Value =
@@ -3177,6 +3183,8 @@
 // using the constructor we found.
 Res = Seq.Perform(S, Entity, Kind, Value);
   }
+
+  return NeedSecondOverloadResolution;
 }
 
 /// Perform the initialization of a potentially-movable value, which
@@ -3201,6 +3209,7 @@
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
+  bool NeedSecondOverloadResolution = true;
 
   if (AllowNRVO) {
 bool AffectedByCWG1579 = false;
@@ -3217,11 +3226,11 @@
 }
 
 if (NRVOCandidate) {
-  TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-true, Res);
+  NeedSecondOverloadResolution = TryMoveInitialization(
+  *this, Entity, NRVOCandidate, ResultType, Value, true, Res);
 }
 
-if (!Res.isInvalid() && AffectedByCWG1579) {
+if (!NeedSecondOverloadResolution && AffectedByCWG1579) {
   QualType QT = NRVOCandidate->getType();
   if (QT.getNonReferenceType()
  .getUnqualifiedType()
@@ -3244,7 +3253,7 @@
 Diag(Value->getExprLoc(), 

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp updated this revision to Diff 311137.
nullptr.cpp added a comment.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=expected %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=expected %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1 &);
+  A1(A1 &&) = delete; // expected-note {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a; // expected-error {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2 &);
+private:
+  A2(A2 &&); // expected-note {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a; // expected-error {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+struct C {};
+
+struct B1 {
+  B1(C &);
+  B1(C &&) = delete; // expected-note {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c; // expected-error {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C &);
+
+private:
+  B2(C &&); // expected-note {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c; // expected-error {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3106,11 +3106,14 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema& S,
-  const InitializedEntity ,
+///
+/// \returns Whether we need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution succeeds but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
-  QualType ResultType,
-  Expr *,
+  QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
   ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
@@ -3123,8 +3126,10 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverloadResolution = true;
+  if (!Seq && Seq.getFailedOverloadResult() != OR_Deleted) {
+return NeedSecondOverloadResolution;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3167,6 +3172,7 @@
   }
 }
 
+NeedSecondOverloadResolution = false;
 // Promote "AsRvalue" to the heap, since we now need this
 // expression node to persist.
 Value =
@@ -3177,6 +3183,8 @@
 // using the constructor we found.
 Res = Seq.Perform(S, Entity, Kind, Value);
   }
+
+  return NeedSecondOverloadResolution;
 }
 
 /// Perform the initialization of a potentially-movable value, which
@@ -3201,6 +3209,7 @@
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
+  bool NeedSecondOverloadResolution = true;
 
   if (AllowNRVO) {
 bool AffectedByCWG1579 = false;
@@ -3217,11 +3226,11 @@
 }
 
 if (NRVOCandidate) {
-  TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
-true, Res);
+  NeedSecondOverloadResolution = TryMoveInitialization(
+  *this, Entity, NRVOCandidate, ResultType, Value, true, Res);
 }
 
-if (!Res.isInvalid() && AffectedByCWG1579) {
+if (!NeedSecondOverloadResolution && AffectedByCWG1579) {
   QualType QT = NRVOCandidate->getType();
   if (QT.getNonReferenceType()
  .getUnqualifiedType()
@@ -3244,7 +3253,7 @@
 Diag(Value->getExprLoc(), 

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3110
+///
+/// \returns Whether need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution success but

s/need/we need/



Comment at: clang/lib/Sema/SemaStmt.cpp:3111
+/// \returns Whether need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution success but
+/// the selected constructor/operator doesn't match the additional criteria, we

s/success/succeeds/



Comment at: clang/lib/Sema/SemaStmt.cpp:3212
   ExprResult Res = ExprError();
+  bool NeedSecondOverload = true;
 

Naming nit: I would prefer to call this "NeedSecondStep" or 
"NeedSecondOverloadResolution" or "NeedSecondResolution". The way it is now, it 
sounds like it's saying we need to find a second //overload//, which isn't 
really the intended meaning.



Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:4
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions 
-verify=cxx11_14_17 %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions 
-verify=cxx11_14_17 %s
+

The new behavior looks good to me (once the tests pass).
Why are there two different `-verify=` tags here, though? This behavior hasn't 
changed between C++11/14/17 and C++20.

(IIUC, the point of this patch is to fix the implementation divergence 
described in 
http://quuxplusone.github.io/draft/d2266-implicit-move-rvalue-ref.html#two-lookups
 , example `thirteen`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92936

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

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

[Yang Fan via Phabricator via cfe-commits]

nullptr.cpp created this revision.
nullptr.cpp requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92936

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

Index: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions -verify=cxx20 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -fcxx-exceptions -verify=cxx11_14_17 %s
+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -fcxx-exceptions -verify=cxx11_14_17 %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify=cxx11_14_17 %s
+
+namespace test_delete_function {
+struct A1 {
+  A1();
+  A1(const A1&);
+  A1(A1&&) = delete;
+  // cxx11_14_17-note@-1 {{'A1' has been explicitly marked deleted here}}
+  // cxx20-note@-2 {{'A1' has been explicitly marked deleted here}}
+};
+A1 test1() {
+  A1 a;
+  return a;
+  // cxx11_14_17-error@-1 {{call to deleted constructor of 'test_delete_function::A1'}}
+  // cxx20-error@-2 {{call to deleted constructor of 'test_delete_function::A1'}}
+}
+
+struct A2 {
+  A2();
+  A2(const A2&);
+private:
+  A2(A2&&);
+  // cxx11_14_17-note@-1 {{declared private here}}
+  // cxx20-note@-2 {{declared private here}}
+};
+A2 test2() {
+  A2 a;
+  return a;
+  // cxx11_14_17-error@-1 {{calling a private constructor of class 'test_delete_function::A2'}}
+  // cxx20-error@-2 {{calling a private constructor of class 'test_delete_function::A2'}}
+}
+
+
+struct C {};
+
+struct B1 {
+  B1(C&);
+  B1(C&&) = delete;
+  // cxx11_14_17-note@-1 {{'B1' has been explicitly marked deleted here}}
+  // cxx20-note@-2 {{'B1' has been explicitly marked deleted here}}
+};
+B1 test3() {
+  C c;
+  return c;
+  // cxx11_14_17-error@-1 {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+  // cxx20-error@-2 {{conversion function from 'test_delete_function::C' to 'test_delete_function::B1' invokes a deleted function}}
+}
+
+struct B2 {
+  B2(C&);
+private:
+  B2(C&&);
+  // cxx11_14_17-note@-1 {{declared private here}}
+  // cxx20-note@-2 {{declared private here}}
+};
+B2 test4() {
+  C c;
+  return c;
+  // cxx11_14_17-error@-1 {{calling a private constructor of class 'test_delete_function::B2'}}
+  // cxx20-error@-2 {{calling a private constructor of class 'test_delete_function::B2'}}
+}
+} // namespace test_delete_function
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3106,11 +3106,14 @@
 /// If move-initialization is not possible, such that we must fall back to
 /// treating the operand as an lvalue, we will leave Res in its original
 /// invalid state.
-static void TryMoveInitialization(Sema& S,
-  const InitializedEntity ,
+///
+/// \returns Whether need to do the second overload resolution. If the first
+/// overload resolution fails, or if the first overload resolution success but
+/// the selected constructor/operator doesn't match the additional criteria, we
+/// need to do the second overload resolution.
+static bool TryMoveInitialization(Sema , const InitializedEntity ,
   const VarDecl *NRVOCandidate,
-  QualType ResultType,
-  Expr *,
+  QualType ResultType, Expr *,
   bool ConvertingConstructorsOnly,
   ExprResult ) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
@@ -3123,8 +3126,10 @@
 
   InitializationSequence Seq(S, Entity, Kind, InitExpr);
 
-  if (!Seq)
-return;
+  bool NeedSecondOverload = true;
+  if (!Seq && Seq.getFailedOverloadResult() != OR_Deleted) {
+return NeedSecondOverload;
+  }
 
   for (const InitializationSequence::Step  : Seq.steps()) {
 if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
@@ -3167,6 +3172,7 @@
   }
 }
 
+NeedSecondOverload = false;
 // Promote "AsRvalue" to the heap, since we now need this
 // expression node to persist.
 Value =
@@ -3177,6 +3183,8 @@
 // using the constructor we found.
 Res = Seq.Perform(S, Entity, Kind, Value);
   }
+
+  return NeedSecondOverload;
 }
 
 /// Perform the initialization of a potentially-movable value, which
@@ -3201,6 +3209,7 @@
   // select the constructor for the copy is first performed as if the object
   // were designated by an rvalue.
   ExprResult Res = ExprError();
+  bool NeedSecondOverload = true;
 
   if (AllowNRVO) {
 bool AffectedByCWG1579 =