[PATCH] D90123: [Sema] Improve notes for value category mismatch in overloading

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

Fixed the spelling issue in rGdea31f135ceae6e860e6301f9bb66d3b3adb1357 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90123

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

[PATCH] D90123: [Sema] Improve notes for value category mismatch in overloading

[Aaron Puchert via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6f84779674a9: [Sema] Improve notes for value category 
mismatch in overloading (authored by aaronpuchert).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90123

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CXX/drs/dr14xx.cpp
  clang/test/CXX/drs/dr1xx.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
  clang/test/SemaCXX/overload-member-call.cpp
  clang/test/SemaCXX/rval-references-examples.cpp

Index: clang/test/SemaCXX/rval-references-examples.cpp
===
--- clang/test/SemaCXX/rval-references-examples.cpp
+++ clang/test/SemaCXX/rval-references-examples.cpp
@@ -13,7 +13,7 @@
 
   ~unique_ptr() { delete ptr; }
 
-  unique_ptr &operator=(unique_ptr &&other) { // expected-note{{candidate function not viable: no known conversion from 'unique_ptr' to 'unique_ptr &&' for 1st argument}}
+  unique_ptr &operator=(unique_ptr &&other) { // expected-note{{candidate function not viable: expects an r-value for 1st argument}}
 if (this == &other)
   return *this;
 
Index: clang/test/SemaCXX/overload-member-call.cpp
===
--- clang/test/SemaCXX/overload-member-call.cpp
+++ clang/test/SemaCXX/overload-member-call.cpp
@@ -83,6 +83,9 @@
 void baz(A &d); // expected-note {{candidate function not viable: 1st argument ('const test1::A') would lose const qualifier}}
 void baz(int i); // expected-note {{candidate function not viable: no known conversion from 'const test1::A' to 'int' for 1st argument}} 
 
+void ref() &&;   // expected-note {{expects an r-value for object argument}} expected-note {{requires 0 arguments, but 1 was provided}}
+void ref(int) &; // expected-note {{expects an l-value for object argument}} expected-note {{requires 1 argument, but 0 were provided}}
+
 // PR 11857
 void foo(int n); // expected-note {{candidate function not viable: requires single argument 'n', but 2 arguments were provided}}
 void foo(unsigned n = 10); // expected-note {{candidate function not viable: allows at most single argument 'n', but 2 arguments were provided}}
@@ -103,6 +106,9 @@
 
 a.rab(); //expected-error {{no matching member function for call to 'rab'}}
 a.zab(3, 4, 5); //expected-error {{no matching member function for call to 'zab'}}
+
+a.ref();// expected-error {{no matching member function for call to 'ref'}}
+A().ref(1); // expected-error {{no matching member function for call to 'ref'}}
   }
 }
 
Index: clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
===
--- clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
+++ clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
@@ -7,7 +7,7 @@
 namespace ClassTemplateParamNotForwardingRef {
   // This is not a forwarding reference.
   template struct A { // expected-note {{candidate}}
-A(T&&); // expected-note {{no known conversion from 'int' to 'int &&'}}
+A(T&&); // expected-note {{expects an r-value}}
   };
   int n;
   A a = n; // expected-error {{no viable constructor or deduction guide}}
@@ -53,8 +53,8 @@
   X xy2 = f0(lvalue());
 }
 
-template X f1(const T&&); // expected-note{{candidate function [with T = int] not viable: no known conversion from 'int' to 'const int &&' for 1st argument}} \
-// expected-note{{candidate function [with T = Y] not viable: no known conversion from 'Y' to 'const Y &&' for 1st argument}}
+template X f1(const T&&); // expected-note{{candidate function [with T = int] not viable: expects an r-value for 1st argument}} \
+// expected-note{{candidate function [with T = Y] not viable: expects an r-value for 1st argument}}
 
 void test_f1() {
   X xi0 = f1(prvalue());
@@ -67,7 +67,7 @@
 
 namespace std_example {
   template  int f(T&&); 
-  template  int g(const T&&); // expected-note{{candidate function [with T = int] not viable: no known conversion from 'int' to 'const int &&' for 1st argument}}
+  template  int g(const T&&); // expected-note{{candidate function [with T = int] not viable: expects an r-value for 1st argument}}
 
   int i;
   int n1 = f(i);
@@ -77,7 +77,7 @@
 #if __cplusplus > 201402L
   template struct A { // expected-note {{candidate}}
 template
-A(T &&, U &&, int *); // expected-note {{[with T = int, U = int] not viable: no known conversion from 'int' to 'int &&'}}
+A(T &&, U &&, int *); // expected-note {{[with T = int, U = int] not viable: expects an r-value}}
 A(T &&, int *);   // expected-note {{requires 2}}
   };
   template A(T &&, int *) -> A; // expe

[PATCH] D90123: [Sema] Improve notes for value category mismatch in overloading

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4280
 "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
-"expects an l-value for "
+"expects an %select{l-value|r-value}5 for "
 "%select{%ordinal4 argument|object argument}3">;

rsmith wrote:
> Please change this to `lvalue|rvalue` (preferably in a separate commit, and 
> likewise for the half-dozen or so other diagnostics that use this 
> unconventional spelling). In both C and C++ contexts, these words are spelled 
> without a hyphen.
I was wondering about the inconsistent spelling as well. Yes, I'll do this in a 
follow-up.



Comment at: clang/lib/Sema/SemaOverload.cpp:10410-10411
 
+  if (Conv.Bad.Kind == BadConversionSequence::lvalue_ref_to_rvalue ||
+  Conv.Bad.Kind == BadConversionSequence::rvalue_ref_to_lvalue) {
+S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_value_category)

rsmith wrote:
> It's surprising to me that nothing else in this function is considering 
> `Conv.Bad.Kind`. Do you know what's going on there? I see that 
> `PerformObjectArgumentInitialization` has a `switch` on it, but it looks like 
> that's the *only* place that uses it, and we mostly instead try to recompute 
> what went wrong after the fact, which seems fragile. I wonder if more of the 
> complexity of this function could be reduced by using the stored bad 
> conversion kind. (But let's keep this patch targeted on just fixing the value 
> category diagnostics!)
The previous `if` could perhaps be on `Conv.Bad.Kind == 
BadConversionSequence::bad_qualifiers` instead, but maybe we're not setting 
that correctly in some cases. I will try it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90123

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

[PATCH] D90123: [Sema] Improve notes for value category mismatch in overloading

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

Thanks!




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4280
 "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
-"expects an l-value for "
+"expects an %select{l-value|r-value}5 for "
 "%select{%ordinal4 argument|object argument}3">;

Please change this to `lvalue|rvalue` (preferably in a separate commit, and 
likewise for the half-dozen or so other diagnostics that use this 
unconventional spelling). In both C and C++ contexts, these words are spelled 
without a hyphen.



Comment at: clang/lib/Sema/SemaOverload.cpp:10410-10411
 
+  if (Conv.Bad.Kind == BadConversionSequence::lvalue_ref_to_rvalue ||
+  Conv.Bad.Kind == BadConversionSequence::rvalue_ref_to_lvalue) {
+S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_value_category)

It's surprising to me that nothing else in this function is considering 
`Conv.Bad.Kind`. Do you know what's going on there? I see that 
`PerformObjectArgumentInitialization` has a `switch` on it, but it looks like 
that's the *only* place that uses it, and we mostly instead try to recompute 
what went wrong after the fact, which seems fragile. I wonder if more of the 
complexity of this function could be reduced by using the stored bad conversion 
kind. (But let's keep this patch targeted on just fixing the value category 
diagnostics!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90123

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

[PATCH] D90123: [Sema] Improve notes for value category mismatch in overloading

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:4927
 // Note that the function case is not possible here.
-if (DeclType->isRValueReferenceType() && LValRefType) {
-  // FIXME: This is the wrong BadConversionSequence. The problem is binding
-  // an rvalue reference to a (non-function) lvalue, not binding an lvalue
-  // reference to an rvalue!
-  ICS.setBad(BadConversionSequence::lvalue_ref_to_rvalue, Init, DeclType);
+if (isRValRef && LValRefType) {
+  ICS.setBad(BadConversionSequence::no_conversion, Init, DeclType);

No functional change here, I was just using the existing prefetched value.



Comment at: clang/lib/Sema/SemaOverload.cpp:4928
+if (isRValRef && LValRefType) {
+  ICS.setBad(BadConversionSequence::no_conversion, Init, DeclType);
   return ICS;

One might think that this should be `rvalue_ref_to_lvalue`, but we have a 
user-defined conversion here, so probably not.



Comment at: clang/lib/Sema/SemaOverload.cpp:10465-10473
-} else if (ToTy->isLValueReferenceType() && !FromExpr->isLValue() &&
-   ToTy.getNonReferenceType().getCanonicalType() ==
-   FromTy.getNonReferenceType().getCanonicalType()) {
-  S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_lvalue)
-  << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << 
FnDesc
-  << (unsigned)isObjectArgument << I + 1
-  << (FromExpr ? FromExpr->getSourceRange() : SourceRange());

We are in the middle of an if-else cascade that is only setting 
`BaseToDerivedConversion`, where I think this doesn't belong. So I moved it 
directly after the qualifier mismatch handling.



Comment at: clang/test/SemaCXX/overload-member-call.cpp:86-87
 
+void ref() &&;   // expected-note {{expects an r-value for object 
argument}} expected-note {{requires 0 arguments, but 1 was provided}}
+void ref(int) &; // expected-note {{expects an l-value for object 
argument}} expected-note {{requires 1 argument, but 0 were provided}}
+

The respective first note was "no known conversion from 'test1::A' to 
'test1::A' for object argument", which is silly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90123

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

[PATCH] D90123: [Sema] Improve notes for value category mismatch in overloading

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, rsmith.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.
aaronpuchert requested review of this revision.

When an overloaded member function has a ref-qualifier, like:

class X {

  void f() &&;
  void f(int) &;

};

we would print strange notes when the ref-qualifier doesn't fit the value
category:

X x;
x.f();
X().f(0);

would both print a note "no known conversion from 'X' to 'X' for object
argument" on their relevant overload instead of pointing out the
mismatch in value category.

At first I thought the solution is easy: just use the FailureKind member
of the BadConversionSequence struct. But it turns out that we weren't
properly setting this for function arguments. So I went through
TryReferenceInit to make sure we're doing that right, and found a number
of notes in the existing tests that improved as well.

Fixes PR47791.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90123

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaOverload.cpp
  clang/test/CXX/drs/dr14xx.cpp
  clang/test/CXX/drs/dr1xx.cpp
  clang/test/CXX/drs/dr6xx.cpp
  clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
  clang/test/SemaCXX/overload-member-call.cpp
  clang/test/SemaCXX/rval-references-examples.cpp

Index: clang/test/SemaCXX/rval-references-examples.cpp
===
--- clang/test/SemaCXX/rval-references-examples.cpp
+++ clang/test/SemaCXX/rval-references-examples.cpp
@@ -13,7 +13,7 @@
 
   ~unique_ptr() { delete ptr; }
 
-  unique_ptr &operator=(unique_ptr &&other) { // expected-note{{candidate function not viable: no known conversion from 'unique_ptr' to 'unique_ptr &&' for 1st argument}}
+  unique_ptr &operator=(unique_ptr &&other) { // expected-note{{candidate function not viable: expects an r-value for 1st argument}}
 if (this == &other)
   return *this;
 
Index: clang/test/SemaCXX/overload-member-call.cpp
===
--- clang/test/SemaCXX/overload-member-call.cpp
+++ clang/test/SemaCXX/overload-member-call.cpp
@@ -83,6 +83,9 @@
 void baz(A &d); // expected-note {{candidate function not viable: 1st argument ('const test1::A') would lose const qualifier}}
 void baz(int i); // expected-note {{candidate function not viable: no known conversion from 'const test1::A' to 'int' for 1st argument}} 
 
+void ref() &&;   // expected-note {{expects an r-value for object argument}} expected-note {{requires 0 arguments, but 1 was provided}}
+void ref(int) &; // expected-note {{expects an l-value for object argument}} expected-note {{requires 1 argument, but 0 were provided}}
+
 // PR 11857
 void foo(int n); // expected-note {{candidate function not viable: requires single argument 'n', but 2 arguments were provided}}
 void foo(unsigned n = 10); // expected-note {{candidate function not viable: allows at most single argument 'n', but 2 arguments were provided}}
@@ -103,6 +106,9 @@
 
 a.rab(); //expected-error {{no matching member function for call to 'rab'}}
 a.zab(3, 4, 5); //expected-error {{no matching member function for call to 'zab'}}
+
+a.ref();// expected-error {{no matching member function for call to 'ref'}}
+A().ref(1); // expected-error {{no matching member function for call to 'ref'}}
   }
 }
 
Index: clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
===
--- clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
+++ clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p3-0x.cpp
@@ -7,7 +7,7 @@
 namespace ClassTemplateParamNotForwardingRef {
   // This is not a forwarding reference.
   template struct A { // expected-note {{candidate}}
-A(T&&); // expected-note {{no known conversion from 'int' to 'int &&'}}
+A(T&&); // expected-note {{expects an r-value}}
   };
   int n;
   A a = n; // expected-error {{no viable constructor or deduction guide}}
@@ -53,8 +53,8 @@
   X xy2 = f0(lvalue());
 }
 
-template X f1(const T&&); // expected-note{{candidate function [with T = int] not viable: no known conversion from 'int' to 'const int &&' for 1st argument}} \
-// expected-note{{candidate function [with T = Y] not viable: no known conversion from 'Y' to 'const Y &&' for 1st argument}}
+template X f1(const T&&); // expected-note{{candidate function [with T = int] not viable: expects an r-value for 1st argument}} \
+// expected-note{{candidate function [with T = Y] not viable: expects an r-value for 1st argument}}
 
 void test_f1() {
   X xi0 = f1(prvalue());
@@ -67,7 +67,7 @@
 
 namespace std_example {
   template  int f(T&&); 
-  template  int g(const T&&); // expected-note{{candidate function [with T = int] not viable: no known conversion from 'int' to 'const int