This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Don't emit "pure virtual" warning for fully qualified calls.
ClosedPublic

Authored by davide on Jul 3 2015, 4:12 PM.

Diff Detail

Event Timeline

davide updated this revision to Diff 29037.Jul 3 2015, 4:12 PM
davide retitled this revision from to [Sema] Don't emit "pure virtual" warning for fully qualified calls..
davide updated this object.
davide added a reviewer: rsmith.
davide set the repository for this revision to rL LLVM.
davide added a subscriber: cfe-commits.
thakis added a subscriber: thakis.Jul 3 2015, 9:26 PM

Qualified calls to virtual methods still go through the vtable in
-fapple-kext mode, so you probably don't want to change that there. (Look
for AppleKext in CodeGen and Sema.)

davide added a comment.Jul 3 2015, 9:59 PM

Interesting, thanks. I didn't know about that. I'll update the patch accordingly tomorrow.

davide updated this revision to Diff 29118.Jul 6 2015, 12:44 PM
davide added a reviewer: thakis.
davide removed rL LLVM as the repository for this revision.
davide removed a subscriber: thakis.

Addressed Nico's comments. While at it added a test.
Nico, I hope this is the right way of checking.

thakis edited edge metadata.Jul 6 2015, 1:24 PM

The kext bits look good to me. I'm not sure if not warning on this at all is a good idea (it looks pretty weird), but the existing warning isn't correct. Maybe the "; overrides of 'f' in subclasses are not available in the constructor of 'A'" should just be omitted in this case? (This is probably up to Richard.)

rsmith edited edge metadata.Jul 6 2015, 1:45 PM

Maybe the "; overrides of 'f' in subclasses are not available in the constructor of 'A'" should just be omitted in this case? (This is probably up to Richard.)

Maybe add a note "qualified call to 'A::f' is treated as a virtual call to 'f' due to -fapple-kext"?

(Also, how about adding "has undefined behavior" before the semicolon in the warning?)

I also wonder if we should add an performsVirtualDispatch(const LangOptions &) to MemberExpr, instead of the somewhat more opaque check for a qualifier here. (There are other places where we perform a similar check, such as MarkExprReferenced and Sema::MarkMemberReferenced, which get the kext case wrong.)

lib/Sema/SemaOverload.cpp
11796

Use hasQualifier instead of getQualifier.

davide updated this revision to Diff 29631.Jul 13 2015, 5:10 PM
davide edited edge metadata.

Addressed Richards' comments.

rsmith accepted this revision.Jul 13 2015, 5:34 PM
rsmith edited edge metadata.

LGTM

include/clang/AST/Expr.h
2584 ↗(On Diff #29631)

Would it make sense for this to also check whether the named member is a virtual function?

include/clang/Basic/DiagnosticSemaKinds.td
1316 ↗(On Diff #29631)

Remove the 'u' from behaviour; our diagnostics are in US English.

This revision is now accepted and ready to land.Jul 13 2015, 5:34 PM
This revision was automatically updated to reflect the committed changes.