[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

rL370737 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Dávid Bolvanský via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370594: [clang] Warning for non-final classes with final 
destructors (authored by xbolva00, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66711?vs=217049&id=218229#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711

Files:
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
  cfe/trunk/test/SemaCXX/warn-final-dtor-non-final-class.cpp


Index: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp
@@ -6236,6 +6236,19 @@
 }
   }
 
+  // Warn if the class has a final destructor but is not itself marked final.
+  if (!Record->hasAttr()) {
+if (const CXXDestructorDecl *dtor = Record->getDestructor()) {
+  if (const FinalAttr *FA = dtor->getAttr()) {
+Diag(FA->getLocation(), diag::warn_final_dtor_non_final_class)
+  << FA->isSpelledAsSealed();
+Diag(Record->getLocation(), 
diag::note_final_dtor_non_final_class_silence)
+  << Context.getRecordType(Record)
+  << FA->isSpelledAsSealed();
+  }
+}
+  }
+
   // See if trivial_abi has to be dropped.
   if (Record->hasAttr())
 checkIllFormedTrivialABIStruct(*Record);
Index: cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
===
--- cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
+++ cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
@@ -440,6 +440,11 @@
 // expected-error@+1 {{base 'SealedType' is marked 'sealed'}}
 struct InheritFromSealed : SealedType {};
 
+class SealedDestructor { // expected-note {{mark 'SealedDestructor' as 
'sealed' to silence this warning}}
+// expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}
+virtual ~SealedDestructor() sealed; // expected-warning {{class with 
destructor marked 'sealed' cannot be inherited from}}
+};
+
 void AfterClassBody() {
   // expected-warning@+1 {{attribute 'deprecated' is ignored, place it after 
"struct" to apply attribute to type declaration}}
   struct D {} __declspec(deprecated);
Index: cfe/trunk/test/SemaCXX/warn-final-dtor-non-final-class.cpp
===
--- cfe/trunk/test/SemaCXX/warn-final-dtor-non-final-class.cpp
+++ cfe/trunk/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s 
-Wfinal-dtor-non-final-class
+
+class A {
+~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+virtual ~B() final; // expected-warning {{class with destructor marked 
'final' cannot be inherited from}}
+};
+
+class C final {
+virtual ~C() final;
+};


Index: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp
@@ -6236,6 +6236,19 @@
 }
   }
 
+  // Warn if the class has a final destructor but is not itself marked final.
+  if (!Record->hasAttr()) {
+if (const CXXDestructorDecl *dtor = Record->getDestructor()) {
+  if (const FinalAttr *FA = dtor->getAttr()) {
+Diag(FA->getLocation(), diag::warn_final_dtor_non_final_class)
+  << FA->isSpelledAsSealed();
+Diag(Record->getLocation(), diag::note_final_dtor_non_final_class_silence)
+  << Context.getRecordType(Record)
+  << FA->isSpelledAsSealed();
+  }
+}
+  }
+
   // See if trivial_abi has to be dropped.
   if (Record->hasAttr())
 checkIllFormedTrivialABIStruct(*Record);
Index: cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
===
--- cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
+++ cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp
@@ -440,6 +440,11 @@
 // expected-error@+1 {{base 'SealedType' is marked 'sealed'}}
 struct InheritFromSealed : SealedType {};
 
+class SealedDestructor { // expected-note {{mark 'SealedDestructor' as 'sealed' to silence this warning}}
+// expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}
+virtual ~SealedDestructor() sealed; // expected-warning {{class with destructor marked 'sealed' cannot be inherited from}}
+};
+
 void AfterClassBody() {
   // expected-warning@+1 {{attribute 'deprecated' is ignored, place it after "struct" to apply attribute to type declaration}}
   struct D {} __declspec(deprecated);
Index: cfe/trunk/test/SemaCXX/warn-final-dtor-non-final-class.cpp
===
--- cfe/trunk/test/SemaCXX/warn-final-dtor-non-final-class.cpp
+++ cfe/trunk/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Logan Smith via Phabricator via cfe-commits]

logan-5 marked an inline comment as done.
logan-5 added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Hiroshi Yamauchi via Phabricator via cfe-commits]

yamauchi accepted this revision.
yamauchi added a comment.

LGTM.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Logan Smith via Phabricator via cfe-commits]

logan-5 updated this revision to Diff 217049.
logan-5 added a comment.

Addressed some feedback.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/MicrosoftExtensions.cpp
  clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp


Index: clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s 
-Wfinal-dtor-non-final-class
+
+class A {
+~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+virtual ~B() final; // expected-warning {{class with destructor marked 
'final' cannot be inherited from}}
+};
+
+class C final {
+virtual ~C() final;
+};
Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -475,6 +475,11 @@
 // expected-error@+1 {{base 'SealedType' is marked 'sealed'}}
 struct InheritFromSealed : SealedType {};
 
+class SealedDestructor { // expected-note {{mark 'SealedDestructor' as 
'sealed' to silence this warning}}
+// expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}
+virtual ~SealedDestructor() sealed; // expected-warning {{class with 
destructor marked 'sealed' cannot be inherited from}}
+};
+
 void AfterClassBody() {
   // expected-warning@+1 {{attribute 'deprecated' is ignored, place it after 
"struct" to apply attribute to type declaration}}
   struct D {} __declspec(deprecated);
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6236,6 +6236,19 @@
 }
   }
 
+  // Warn if the class has a final destructor but is not itself marked final.
+  if (!Record->hasAttr()) {
+if (const CXXDestructorDecl *dtor = Record->getDestructor()) {
+  if (const FinalAttr *FA = dtor->getAttr()) {
+Diag(FA->getLocation(), diag::warn_final_dtor_non_final_class)
+  << FA->isSpelledAsSealed();
+Diag(Record->getLocation(), 
diag::note_final_dtor_non_final_class_silence)
+  << Context.getRecordType(Record)
+  << FA->isSpelledAsSealed();
+  }
+}
+  }
+
   // See if trivial_abi has to be dropped.
   if (Record->hasAttr())
 checkIllFormedTrivialABIStruct(*Record);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2201,6 +2201,11 @@
   "base %0 is marked '%select{final|sealed}1'">;
 def warn_abstract_final_class : Warning<
   "abstract class is marked '%select{final|sealed}0'">, 
InGroup;
+def warn_final_dtor_non_final_class : Warning<
+  "class with destructor marked '%select{final|sealed}0' cannot be inherited 
from">,
+  InGroup;
+def note_final_dtor_non_final_class_silence : Note<
+  "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -114,6 +114,7 @@
  [DeleteNonAbstractNonVirtualDtor,
   DeleteAbstractNonVirtualDtor]>;
 def AbstractFinalClass : DiagGroup<"abstract-final-class">;
+def FinalDtorNonFinalClass : DiagGroup<"final-dtor-non-final-class">;
 
 def CXX11CompatDeprecatedWritableStr :
   DiagGroup<"c++11-compat-deprecated-writable-strings">;


Index: clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s -Wfinal-dtor-non-final-class
+
+class A {
+~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+virtual ~B() final; // expected-warning {{class with destructor marked 'final' cannot be inherited from}}
+};
+
+class C final {
+virtual ~C() final;
+};
Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -475,6 +475,11 @@
 // expected-error@+1 {{base 'SealedType' is marked

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2204
   "abstract class is marked '%select{final|sealed}0'">, 
InGroup;
+def warn_final_destructor_nonfinal_class : Warning<
+  "class with destructor marked '%select{final|sealed}0' cannot be inherited 
from">,

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.



Comment at: clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp:12
+class C final {
+virtual ~C() final;
+};

Should there be a test for the `sealed` spelling?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 accepted this revision.
xbolva00 added a comment.
This revision is now accepted and ready to land.

Looks fine


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66711/new/

https://reviews.llvm.org/D66711



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

[Logan Smith via Phabricator via cfe-commits]

logan-5 created this revision.
logan-5 added reviewers: rsmith, yamauchi.
logan-5 added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D66711

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp


Index: clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s 
-Wfinal-dtor-non-final-class
+
+class A {
+~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+virtual ~B() final; // expected-warning {{class with destructor marked 
'final' cannot be inherited from}}
+};
+
+class C final {
+virtual ~C() final;
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6235,6 +6235,19 @@
 }
   }
 
+  // Warn if the class has a final destructor but is not itself marked final.
+  if (!Record->hasAttr()) {
+if (CXXDestructorDecl *dtor = Record->getDestructor()) {
+  if (FinalAttr *FA = dtor->getAttr()) {
+Diag(FA->getLocation(), diag::warn_final_destructor_nonfinal_class)
+  << FA->isSpelledAsSealed();
+Diag(Record->getLocation(), 
diag::note_final_destructor_nonfinal_class_silence)
+  << Context.getRecordType(Record)
+  << FA->isSpelledAsSealed();
+  }
+}
+  }
+
   // See if trivial_abi has to be dropped.
   if (Record->hasAttr())
 checkIllFormedTrivialABIStruct(*Record);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2201,6 +2201,11 @@
   "base %0 is marked '%select{final|sealed}1'">;
 def warn_abstract_final_class : Warning<
   "abstract class is marked '%select{final|sealed}0'">, 
InGroup;
+def warn_final_destructor_nonfinal_class : Warning<
+  "class with destructor marked '%select{final|sealed}0' cannot be inherited 
from">,
+  InGroup;
+def note_final_destructor_nonfinal_class_silence : Note<
+  "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -114,6 +114,7 @@
  [DeleteNonAbstractNonVirtualDtor,
   DeleteAbstractNonVirtualDtor]>;
 def AbstractFinalClass : DiagGroup<"abstract-final-class">;
+def FinalDtorNonFinalClass : DiagGroup<"final-dtor-non-final-class">;
 
 def CXX11CompatDeprecatedWritableStr :
   DiagGroup<"c++11-compat-deprecated-writable-strings">;


Index: clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s -Wfinal-dtor-non-final-class
+
+class A {
+~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+virtual ~B() final; // expected-warning {{class with destructor marked 'final' cannot be inherited from}}
+};
+
+class C final {
+virtual ~C() final;
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6235,6 +6235,19 @@
 }
   }
 
+  // Warn if the class has a final destructor but is not itself marked final.
+  if (!Record->hasAttr()) {
+if (CXXDestructorDecl *dtor = Record->getDestructor()) {
+  if (FinalAttr *FA = dtor->getAttr()) {
+Diag(FA->getLocation(), diag::warn_final_destructor_nonfinal_class)
+  << FA->isSpelledAsSealed();
+Diag(Record->getLocation(), diag::note_final_destructor_nonfinal_class_silence)
+  << Context.getRecordType(Record)
+  << FA->isSpel