This is an archive of the discontinued LLVM Phabricator instance.

[clang] Improve diagnostics on implicitly deleted defaulted comparisons
ClosedPublic

Authored by mizvekov on Mar 4 2021, 4:30 PM.

Details

Summary

This patch just makes the error message clearer by reinforcing the cause
was a lack of viable three-way comparison function for the
complete object.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov requested review of this revision.Mar 4 2021, 4:30 PM
mizvekov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 4:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov edited the summary of this revision. (Show Details)Mar 4 2021, 4:51 PM

I don't think this change is right: for

struct A {
  int &r;
  bool operator==(const A&) const = default;
};

we should warn about the reference member (as we do), but for

struct A {
  int &r;
  bool operator<(const A&) const = default;
};

... we shouldn't recurse to subobjects because they make no difference. The problem in that example is that there's no operator<=> for A; the fact that there's a reference member is irrelevant. I think it would be useful to change the diagnostic from

<stdin>:3:8: warning: explicitly defaulted relational comparison operator is implicitly deleted [-Wdefaulted-function-deleted]
  bool operator<(const A &) const = default;
       ^
<stdin>:3:8: note: defaulted 'operator<' is implicitly deleted because there is no viable comparison function

to something more verbose, such as

<stdin>:3:8: warning: explicitly defaulted relational comparison operator is implicitly deleted [-Wdefaulted-function-deleted]
  bool operator<(const A &) const = default;
       ^
<stdin>:3:8: note: defaulted 'operator<' is implicitly deleted because there is no viable three-way comparison function for 'A'
mizvekov updated this revision to Diff 329178.Mar 8 2021, 6:11 PM

Now this a patch just about improving the diagnostic as rsmith suggested.

mizvekov retitled this revision from [clang] Always provide relevant subobject for 'no viable comparison' to [clang] Improve diagnostics on implicitly deleted defaulted comparisons.Mar 8 2021, 6:12 PM
mizvekov edited the summary of this revision. (Show Details)

You are absolutely right, my bad!!

So we stealth fix this revision to improve the message :D

mizvekov updated this revision to Diff 329289.Mar 9 2021, 4:07 AM

Change to simpler way to do it:
For CompleteObject Kind, just fill in the declaration for the complete object.

rsmith accepted this revision.Mar 12 2021, 1:48 PM
rsmith added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8968–8970 ↗(On Diff #329289)

Would it be useful to apply the same diagnostic improvement to this diagnostic too? (Genuine question: I *think* we'll attach a note pointing to the deleted function in this case, which would probably make the "for T" part just be noise, but I've not checked.)

This revision is now accepted and ready to land.Mar 12 2021, 1:48 PM
mizvekov added inline comments.Mar 12 2021, 3:21 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8968–8970 ↗(On Diff #329289)

Yeah I thought about checking other errors for similar improvements. It's on my list to check that.

mizvekov added inline comments.Mar 13 2021, 6:24 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8968–8970 ↗(On Diff #329289)

For this one in particular, we really do attach another note that makes it obvious, pointing to the explicitly marked deleted function.
note_defaulted_comparison_not_constexpr at first glance looks like might suffer from the same problem, but in this case I don't think the error ever could apply to the complete object.
And then nothing else that I could find.