Page MenuHomePhabricator

Fix inconsistent flag for disabling Dead Virtual Function Elimination
ClosedPublic

Authored by ddcc on Fri, Sep 25, 6:50 PM.

Diff Detail

Event Timeline

ddcc created this revision.Fri, Sep 25, 6:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 25, 6:50 PM
Herald added a subscriber: dang. · View Herald Transcript
ddcc requested review of this revision.Fri, Sep 25, 6:50 PM

Can you add a test? You can probably just beef up clang/test/CodeGenCXX/virtual-function-elimination.cpp to test this option as well

ddcc updated this revision to Diff 296782.Wed, Oct 7, 1:23 PM

Add test

tejohnson accepted this revision.Wed, Oct 7, 1:51 PM

LGTM with the test change suggested below.

clang/test/CodeGenCXX/virtual-function-elimination.cpp
2

Since VFE isn't the default, I suspect you need to put -fvirtual-function-elimination before the -fno-virtual-function-elimination for it to really test that it is disabling VFE. (Although I realize that this does test this patch fix since presumably it complained about the unknown option before fixing it, but still it would be good to ensure that we're also testing that the -fno- form has the desired affect.)

This revision is now accepted and ready to land.Wed, Oct 7, 1:51 PM
ddcc updated this revision to Diff 296826.Wed, Oct 7, 4:18 PM

Update tests

ddcc added a comment.Wed, Oct 7, 4:18 PM

Hmm, looks like this was already fixed by e5158b52730d323bb8cd2cba6dc6c89b90cba452. I guess I'll just commit the test then?

tejohnson accepted this revision.Wed, Oct 7, 4:41 PM

LGTM for test - thanks! Yeah, it looks like that cleanup patch inadvertently fixed this issue as a side effect.

This revision was landed with ongoing or failed builds.Wed, Oct 7, 4:43 PM
This revision was automatically updated to reflect the committed changes.