[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