This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Improve notes for value category mismatch in overloading
ClosedPublic

Authored by aaronpuchert on Oct 25 2020, 10:09 AM.

Details

Summary

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.

Diff Detail

Event Timeline

aaronpuchert created this revision.Oct 25 2020, 10:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2020, 10:09 AM
aaronpuchert requested review of this revision.Oct 25 2020, 10:09 AM
aaronpuchert added inline comments.Oct 25 2020, 10:26 AM
clang/lib/Sema/SemaOverload.cpp
4971–4972

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

4972

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

10535–10543

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.

clang/test/SemaCXX/overload-member-call.cpp
86–87

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

rsmith accepted this revision.Oct 29 2020, 2:38 PM

Thanks!

clang/include/clang/Basic/DiagnosticSemaKinds.td
4292

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.

clang/lib/Sema/SemaOverload.cpp
10480–10481

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!)

This revision is now accepted and ready to land.Oct 29 2020, 2:38 PM
aaronpuchert added inline comments.Nov 15 2020, 7:54 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
4292

I was wondering about the inconsistent spelling as well. Yes, I'll do this in a follow-up.

clang/lib/Sema/SemaOverload.cpp
10480–10481

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.

aaronpuchert marked an inline comment as done.Nov 15 2020, 9:07 AM

Fixed the spelling issue in rGdea31f135ceae6e860e6301f9bb66d3b3adb1357.