Page MenuHomePhabricator

[clang] Warning for non-final classes with final destructors
ClosedPublic

Authored by logan-5 on Sat, Aug 24, 7:55 PM.

Details

Summary

Marking a class' destructor final prevents the class from being inherited from. However, it is a subtle and awkward way to express that at best, and unintended at worst. It may also generate worse code (in other compilers) than marking the class itself final. For these reasons, this revision adds a warning for nonfinal classes with final destructors, with a note to suggest marking the class final to silence the warning.

See https://reviews.llvm.org/D66621 for more background.

Patch by logan-5 (Logan Smith)

Diff Detail

Repository
rL LLVM

Event Timeline

logan-5 created this revision.Sat, Aug 24, 7:55 PM
xbolva00 accepted this revision.Sun, Aug 25, 7:42 AM
xbolva00 added a subscriber: xbolva00.

Looks fine

This revision is now accepted and ready to land.Sun, Aug 25, 7:42 AM
Quuxplusone added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
2204 ↗(On Diff #217032)

Is there any good reason to spell it dtor in the preceding file but destructor in this one? I think the spelling should be consistent one way or the other. Helps with greppability/maintainability.

clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp
12 ↗(On Diff #217032)

Should there be a test for the sealed spelling?

logan-5 updated this revision to Diff 217049.Sun, Aug 25, 9:20 AM

Addressed some feedback.

logan-5 marked an inline comment as done.Tue, Aug 27, 11:50 AM

Thanks. I'll need someone with commit access to help me out with this.

xbolva00 edited the summary of this revision. (Show Details)Wed, Aug 28, 10:10 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSat, Aug 31, 11:30 AM

This is a cool warning. Could the note have a fixit attached to it, so that it's easy to mass-clean-up the warning automatically?