Page MenuHomePhabricator

New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor
Needs ReviewPublic

Authored by ArnaudBienner on Jan 6 2019, 1:05 PM.

Details

Summary

Adding you, Nico, as a reviewer, since you worked on a similar warning, and reviewed my recent changes.
Feel free to add/delegate to someone else.

Diff Detail

Event Timeline

ArnaudBienner created this revision.Jan 6 2019, 1:05 PM
ArnaudBienner edited the summary of this revision. (Show Details)Jan 6 2019, 1:17 PM
ArnaudBienner added a reviewer: thakis.
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 9:01 AM
rjmccall added inline comments.Jun 27 2019, 11:25 PM
lib/Sema/SemaOverload.cpp
13075

Intentionally suppressed in lambdas? I think that might be reasonable, but also worth a comment.

13080

This should be folded into the same checks that lead into that warning, since most of the conditions are identical and it's just a few final decisions (and the purity of the function) that changes whether to emit a diagnostic and which one.

13087

Use cast<> instead of dyn_cast+assert.

13091

ContextRD must be polymorphic, it provides a virtual method.

13099

As a matter of general principle, these diagnostics should be at TheCall->getExprLoc() instead of MemExpr->getBeginLoc(). As a matter of correctness, the insertion must start at MemExpr->getMemberLoc() instead of MemExpr->getBeginLoc() because the base in the member expression might be explicit.

Please add tests for the fix-its.

Thanks @xbolva00 for adding reviewers and @rjmccall for reviewing!

@rjmccall as you might remember, we had a discussion on the mailing list ("Warning when calling virtual functions from constructor/desctructor?") and from what I understand the overall feeling was that this patch/warning won't be accepted until we move forward with your proposal of having a "-Wversion=" flag to allow deactivate new warnings when upgrading clang, but enable them by default otherwise.
Have you made any progress on that? Or do you think the warning can be implemented anyway? (maybe off by default?)

Just want to avoid spending our time reviewing/doing changes on a patch that won't be accepted in the end :)

Oh, sorry, I had completely forgotten about that.

My contributions to Clang are primarily code review these days; I am not working on that idea.