This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by logan-5 on Aug 24 2019, 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
rC Clang

Event Timeline

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

Looks fine

This revision is now accepted and ready to land.Aug 25 2019, 7:42 AM
Quuxplusone added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
2204

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.Aug 25 2019, 9:20 AM

Addressed some feedback.

logan-5 marked an inline comment as done.Aug 27 2019, 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)Aug 28 2019, 10:10 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2019, 11:30 AM
thakis added a subscriber: thakis.Sep 2 2019, 3:18 PM

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?