This is an archive of the discontinued LLVM Phabricator instance.

[clang] Devirtualization for classes with destructors marked as 'final'
ClosedPublic

Authored by logan-5 on Aug 22 2019, 2:19 PM.

Details

Summary

A class with a destructor marked final cannot be derived from, so it should afford the same devirtualization opportunities as marking the entire class final.

Diff Detail

Repository
rC Clang

Event Timeline

logan-5 created this revision.Aug 22 2019, 2:19 PM

That appears sort of tangential to me. To clarify, this PR is not (necessarily) about devirtualizing destructors themselves, but rather devirtualizing OTHER member function calls for classes whose destructor is marked final, since that is sort of morally equivalent to marking the entire class final.

rsmith accepted this revision.Aug 22 2019, 5:58 PM

This seems subtle, but I believe it is correct.

I wonder whether we should provide a warning for a non-final class has a final destructor, since moving the final from the destructor to the class seems like a more obvious way to present the code (and will likely lead to better code generation in compilers that haven't realized they can do this).

clang/lib/AST/DeclCXX.cpp
2074

This comment doesn't make much sense (pre-existing, but since you're changing it anyway): classes can't be "overridden", they can only be derived from.

This revision is now accepted and ready to land.Aug 22 2019, 5:58 PM
logan-5 updated this revision to Diff 216757.Aug 22 2019, 6:52 PM

Tweaked comment wording for accuracy.

logan-5 marked an inline comment as done.Aug 22 2019, 6:57 PM

@rsmith I agree having a final destructor in a non-final class smells weird. I'd be interested in working on adding a warning like that, if it sounds like a useful thing.

For now, I need help committing this, if anyone would be so kind!

Nice.

This seems subtle, but I believe it is correct.

I wonder whether we should provide a warning for a non-final class has a final destructor, since moving the final from the destructor to the class seems like a more obvious way to present the code (and will likely lead to better code generation in compilers that haven't realized they can do this).

Richard, do you think there may be some missed devirtualization optimizations that would trigger if the class, rather than the destructor, is declared final in Clang? It seems this patch would take care of common call devirtualizations.

logan-5 updated this revision to Diff 217009.Aug 23 2019, 10:16 PM

Add a missing null check.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2019, 11:51 AM

For now, I need help committing this, if anyone would be so kind!

rL370597