Page MenuHomePhabricator

[Sema] Improve diagnostics for const- and ref-qualified member functions
ClosedPublic

Authored by jtbandes on Nov 11 2017, 12:31 AM.

Details

Summary

Adjust wording for const-qualification mismatch to be a little more clear.

Also add another diagnostic for a ref qualifier mismatch, which previously produced a useless error (this error path is simply very old; see rL119336):

Before:

error: cannot initialize object parameter of type 'X0' with an expression of type 'X0'

After:

error: 'this' argument to member function 'rvalue' is an lvalue, but function has rvalue ref-qualifier

Diff Detail

Repository
rC Clang

Event Timeline

jtbandes created this revision.Nov 11 2017, 12:31 AM
aaron.ballman added a subscriber: aaron.ballman.

I think the new wording is a bit more clear, in general.

include/clang/Basic/DiagnosticSemaKinds.td
1602–1604

I don't think this diagnostic needs to be moved and renamed; the old name was more clear than "other" on the new name.

test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
22–24

Can you add examples that cover the other diagnostic wordings as well (volatile, restrict, combinations, etc)?

jtbandes added inline comments.Nov 15 2017, 12:00 PM
include/clang/Basic/DiagnosticSemaKinds.td
1602–1604

How about _bad_type? I was hoping to give these all similar names since they happen during the same implicit conversion. Also, while "implicit object parameter" is technically a correct name, it seems to be not very commonly used and would confuse users. Though it's also valuable for the diagnostic id to be similar to the actual text...

aaron.ballman added inline comments.Nov 15 2017, 12:46 PM
include/clang/Basic/DiagnosticSemaKinds.td
1602–1604

I'm not too worried about the identifier used for the diagnostic; it doesn't seem likely to confuse many people working on Clang (or at least, not confuse them for long). However, I wouldn't be opposed to err_member_function_call_bad_type.

jtbandes added inline comments.Nov 16 2017, 9:06 PM
test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
22–24

I've been working on this, but I actually can't trigger the restrict variants. Do you know whether this is something that's expected to work? The implicit object param doesn't seem to retain its restrict-ness (full disclosure, I have almost no prior experience with restrict...):

void c() const;
void v() volatile;
void r() __restrict__;
void cr() const __restrict__;
void cv() const volatile;
void vr() volatile __restrict__;
void cvr() const volatile __restrict__;
void test_diagnostics(const volatile X0 &__restrict__ cvr) {
  cvr.g(); // expected-error {{not marked const, volatile, or restrict}}  -- actually produces "not marked const or volatile"
  cvr.c(); // expected-error {{not marked volatile or restrict}}  -- actually produces "not marked volatile"
  cvr.v(); // expected-error {{not marked const or restrict}}  -- actually produces "not marked const"
  cvr.r(); // expected-error {{not marked const or volatile}}
  cvr.cr(); // expected-error {{not marked volatile}}
  cvr.cv(); // expected-error {{not marked restrict}}  -- actually produces no error
  cvr.vr(); // expected-error {{not marked const}}
  cvr.cvr();
}
aaron.ballman added inline comments.Nov 18 2017, 11:48 AM
test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
22–24

Given that restrict is a Cism, it's entirely possible that it's not really supported as a member function qualifier. In that case, one more test for const volatile should be sufficient.

lebedev.ri added inline comments.
test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
22–24

Note that you might want to test it regardless, to not miss if it starts to warn for some reason.

jtbandes updated this revision to Diff 123476.Nov 18 2017, 2:06 PM
  • feedback from review & more tests
jtbandes marked 2 inline comments as done.Nov 18 2017, 2:11 PM
aaron.ballman added inline comments.Nov 18 2017, 2:30 PM
test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
85

You should spell out the entire diagnostic at least the first time it appears in the file.

jtbandes updated this revision to Diff 123481.Nov 18 2017, 2:33 PM
  • spell out full diagnostic the first time
jtbandes marked an inline comment as done.Nov 18 2017, 2:34 PM
aaron.ballman accepted this revision.Nov 18 2017, 2:57 PM

This LGTM, but you should wait for a few days before committing in case @rsmith has comments.

This revision is now accepted and ready to land.Nov 18 2017, 2:57 PM

Thanks, will do. Is there an automated system that can run all the tests before I merge rather than waiting for a potential build failure after the fact?

Thanks, will do. Is there an automated system that can run all the tests before I merge rather than waiting for a potential build failure after the fact?

You can run the tests locally using llvm-lit, which will get you some coverage for your platform and architecture, but that's about the best you can do locally.

https://llvm.org/docs/TestingGuide.html#id1

You're set to commit (if there are concerns, @rsmith can raise them post-commit). Do you need me to commit on your behalf?

This revision was automatically updated to reflect the committed changes.

After merging the buildbots informed me some other tests were broken that I failed to notice. I reverted this commit and submitted D41646 which reintroduces the changes and fixes the other broken tests.