[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

In D82728#2161152 , @xbolva00 wrote:

> Is it possible to emit fixit note with "override" ?


This is a good idea, though unfortunately (after eyeballing the implementation 
of `modernize-use-override` in clang-tidy (UseOverrideCheck.cpp)), it looks 
non-trivial to figure out where exactly to insert `override`. There's some 
significant logic in the clang-tidy check involving re-lexing the relevant 
tokens, to find the insertion point in the presence of complexity like inline 
definitions, `= 0`, `= {delete|default}`, function try blocks, macros, and the 
list goes on.

The clang-tidy check has this FIXME comment to address the complexity 
(UseOverrideCheck.cpp:136):

  // FIXME: Instead of re-lexing and looking for specific macros such as
  // 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each
  // FunctionDecl.

If this was done, the resulting information would presumably simplify the 
clang-tidy check as well as make it easier to add a fixit to this warning. This 
sounds like an interesting problem, but also like a sizable refactor, and I 
won't be upset if someone beats me to it. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

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

xbolva00 added a comment.

Is it possible to emit fixit note with "override" ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG67895d47: [clang] Add -Wsuggest-override (authored by 
logan-5, committed by dblaikie).

Changed prior to commit:
  https://reviews.llvm.org/D82728?vs=276248&id=277308#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+struct A {
+  ~A();
+  void run();
+};
+
+struct B : public A {
+  ~B();
+  void run();
+};
+
+struct C {
+  virtual void run(); // expected-note 2{{overridden virtual function is here}}
+  virtual ~C();
+};
+
+struct D : public C {
+  void run();
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D();
+};
+
+struct E : public C {
+  virtual void run();
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E();
+};
+
+struct F : public C {
+  void run() override;
+  ~F() override;
+};
+
+struct G : public C {
+  void run() final;
+  ~G() final;
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+struct A {
+  ~A();
+  virtual void run();
+};
+
+struct B : public A {
+  ~B();
+};
+
+struct C {
+  virtual void run();
+  virtual ~C();  // expected-note 2{{overridden virtual function is here}}
+};
+
+struct D : public C {
+  void run();
+  ~D();
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+struct E : public C {
+  void run();
+  virtual ~E();
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,22 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [&](unsigned DiagInconsistent, unsigned DiagSuggest) {
+  unsigned DiagID =
+  Inconsistent && !Diags.isIgnored(DiagInconsistent, MD->getLocation())
+  ? DiagInconsistent
+  : DiagSuggest;
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (isa(MD))
+  EmitDiag(
+  diag::warn_inconsistent_destructor_marked_not_override_overriding,
+  diag::warn_suggest_destructor_marked_not_override_overriding);
+else
+  EmitDiag(diag::warn_inconsistent_function_marked_not_override_overriding,
+   diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
@@ -6749,13 +6759,10 @@
 }
   }
 
-  if (HasMethodWithOverrideControl &&
-  HasOverridingMethodWithoutOverrideControl) {
-// At least one method has the 'override' control declared.
-// Diagnose all other overridden methods which do not have 'override'
-// specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool HasInconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())
-  DiagnoseAbsenceOfOverrideControl(M);
+  DiagnoseAbsenceOfOverrideControl(M, HasInconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/S

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

In D82728#2146067 , @dblaikie wrote:

> Looks good - thanks for the patch and all the details! Might be worth turning 
> on by default in the LLVM build (after all the cleanup)


Thanks a lot! I don't (think I) have commit access yada yada, so I'd really 
appreciate any help getting this committed.

I've already got some of the cleanup in the works (D83611 
 and D83616 
), and was planning on taking care of the rest 
throughout the coming weekish. I'd be happy to turn this warning on in the LLVM 
build after that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

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

In D82728#2142071 , @logan-5 wrote:

> Feels like a dumb question, but I'm not sure if and how those build failures 
> are related to this patch? They seem to involve completely separate parts of 
> the LLVM project (and `.c` files to boot). Was it that the build was failing 
> for an unrelated reason at the moment the bots happened to build this patch? 
> Or am I misunderstanding something more obvious?


Yeah, don't think those are related, no. Wouldn't worry about them.

Looks good - thanks for the patch and all the details! Might be worth turning 
on by default in the LLVM build (after all the cleanup)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

Feels like a dumb question, but I'm not sure if and how those build failures 
are related to this patch? They seem to involve completely separate parts of 
the LLVM project (and `.c` files to boot). Was it that the build was failing 
for an unrelated reason at the moment the bots happened to build this patch? Or 
am I misunderstanding something more obvious?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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



Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:1
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify 
-Wsuggest-destructor-override
+

logan-5 wrote:
> dblaikie wrote:
> > Does GCC have suggest-destructor-override as a separate warning too?
> It does not. In fact, there's no way to get GCC to warn about destructors 
> missing `override`. (Which is somewhat defensible, since `override` is really 
> great for preventing subtle signature mismatches/typos, but destructors don't 
> really have those problems.) However, Clang already has a destructor-specific 
> flavor of the inconsistent-override warning, so I added 
> -Wsuggest-destructor-override for symmetry with 
> -Winconsistent-missing-[destructor-]override.
Note that this really is the best of both worlds, since it lets Clang's 
-Wsuggest-override behave identically to GCC's (not warning on destructors), as 
well as be consistent with its own already existing override warnings (with a 
special extra flag to enable warnings for destructors).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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



Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:1
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify 
-Wsuggest-destructor-override
+

dblaikie wrote:
> Does GCC have suggest-destructor-override as a separate warning too?
It does not. In fact, there's no way to get GCC to warn about destructors 
missing `override`. (Which is somewhat defensible, since `override` is really 
great for preventing subtle signature mismatches/typos, but destructors don't 
really have those problems.) However, Clang already has a destructor-specific 
flavor of the inconsistent-override warning, so I added 
-Wsuggest-destructor-override for symmetry with 
-Winconsistent-missing-[destructor-]override.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Generally looks good to me (description/commit message should be updated now 
that the inconsistent inconsistency is no longer an issue)




Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:1
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify 
-Wsuggest-destructor-override
+

Does GCC have suggest-destructor-override as a separate warning too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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

Fall back to -Wsuggest-override if -Winconsistent-missing-override is disabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+struct A {
+  ~A();
+  void run();
+};
+
+struct B : public A {
+  ~B();
+  void run();
+};
+
+struct C {
+  virtual void run(); // expected-note 2{{overridden virtual function is here}}
+  virtual ~C();
+};
+
+struct D : public C {
+  void run();
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D();
+};
+
+struct E : public C {
+  virtual void run();
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E();
+};
+
+struct F : public C {
+  void run() override;
+  ~F() override;
+};
+
+struct G : public C {
+  void run() final;
+  ~G() final;
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+struct A {
+  ~A();
+  virtual void run();
+};
+
+struct B : public A {
+  ~B();
+};
+
+struct C {
+  virtual void run();
+  virtual ~C();  // expected-note 2{{overridden virtual function is here}}
+};
+
+struct D : public C {
+  void run();
+  ~D();
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+struct E : public C {
+  void run();
+  virtual ~E();
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,22 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [&](unsigned DiagInconsistent, unsigned DiagSuggest) {
+  unsigned DiagID =
+  Inconsistent && !Diags.isIgnored(DiagInconsistent, MD->getLocation())
+  ? DiagInconsistent
+  : DiagSuggest;
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (isa(MD))
+  EmitDiag(
+  diag::warn_inconsistent_destructor_marked_not_override_overriding,
+  diag::warn_suggest_destructor_marked_not_override_overriding);
+else
+  EmitDiag(diag::warn_inconsistent_function_marked_not_override_overriding,
+   diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
@@ -6749,13 +6759,10 @@
 }
   }
 
-  if (HasMethodWithOverrideControl &&
-  HasOverridingMethodWithoutOverrideControl) {
-// At least one method has the 'override' control declared.
-// Diagnose all other overridden methods which do not have 'override'
-// specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool HasInconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())
-  DiagnoseAbsenceOfOverrideControl(M);
+  DiagnoseAbsenceOfOverrideControl(M, HasInconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6961,7 +6961,7 @@
 
   /// DiagnoseAbsenceOfOverrideControl - Diagnose if 'override' keyword was
   /// no

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

In D82728#2137492 , @dblaikie wrote:

> Oh, yep, there's a way - it's usually used for performance-costly warnings, 
> to not spend the resources computing the warning if it's disabled anyway.


Wow, thanks--overlooked that in a big way. That'll definitely solve the 
problem. Fixed up patch on the way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D82728#2137061 , @logan-5 wrote:

> In D82728#2137021 , @dblaikie wrote:
>
> > I think it might be nice to make the -Wno-inconsistent-missing-override 
> > -Wsuggest-override situation a bit better (by having it still do the same 
> > thing as -Wsuggest-override) but I don't feel /too/ strongly about it.
>
>
> So, ironing this out would mean the code would need this structure:
>
>   if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override))
>Emit(-Winconsistent-missing-override);
>else
>Emit(-Wsuggest-override);
>
> The issue is that I wasn't able to find a way to ask if a warning is enabled 
> and make a control flow decision based on that. If there is an API for doing 
> this, it's hidden well. :) So I fell back to just doing `if (Inconsistent)` 
> instead, which has this quirk if the inconsistent version of the warning is 
> disabled.


Oh, yep, there's a way - it's usually used for performance-costly warnings, to 
not spend the resources computing the warning if it's disabled anyway.

Here's an arbitrary example: 
https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7237


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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

Addressed minor feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+struct A {
+  ~A();
+  void run();
+};
+
+struct B : public A {
+  ~B();
+  void run();
+};
+
+struct C {
+  virtual void run(); // expected-note 2{{overridden virtual function is here}}
+  virtual ~C();
+};
+
+struct D : public C {
+  void run();
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D();
+};
+
+struct E : public C {
+  virtual void run();
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E();
+};
+
+struct F : public C {
+  void run() override;
+  ~F() override;
+};
+
+struct G : public C {
+  void run() final;
+  ~G() final;
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+struct A {
+  ~A();
+  virtual void run();
+};
+
+struct B : public A {
+  ~B();
+};
+
+struct C {
+  virtual void run();
+  virtual ~C();  // expected-note 2{{overridden virtual function is here}}
+};
+
+struct D : public C {
+  void run();
+  ~D();
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+struct E : public C {
+  void run();
+  virtual ~E();
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,19 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [&](unsigned DiagDtor, unsigned DiagFn) {
+  unsigned DiagID = isa(MD) ? DiagDtor : DiagFn;
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (Inconsistent)
+  EmitDiag(
+  diag::warn_inconsistent_destructor_marked_not_override_overriding,
+  diag::warn_inconsistent_function_marked_not_override_overriding);
+else
+  EmitDiag(diag::warn_suggest_destructor_marked_not_override_overriding,
+   diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
@@ -6749,13 +6756,10 @@
 }
   }
 
-  if (HasMethodWithOverrideControl &&
-  HasOverridingMethodWithoutOverrideControl) {
-// At least one method has the 'override' control declared.
-// Diagnose all other overridden methods which do not have 'override'
-// specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool HasInconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())
-  DiagnoseAbsenceOfOverrideControl(M);
+  DiagnoseAbsenceOfOverrideControl(M, HasInconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6961,7 +6961,7 @@
 
   /// DiagnoseAbsenceOfOverrideControl - Diagnose if 'override' keyword was
   /// not used in the declaration of an overriding method.
-  void DiagnoseAbsenceOfOverrideControl(NamedDecl *D);
+  void DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsisten

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 marked 2 inline comments as done.
logan-5 added a comment.

In D82728#2137021 , @dblaikie wrote:

> I think it might be nice to make the -Wno-inconsistent-missing-override 
> -Wsuggest-override situation a bit better (by having it still do the same 
> thing as -Wsuggest-override) but I don't feel /too/ strongly about it.


So, ironing this out would mean the code would need this structure:

  if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override))
  Emit(-Winconsistent-missing-override);
  else
  Emit(-Wsuggest-override);

The issue is that I wasn't able to find a way to ask if a warning is enabled 
and make a control flow decision based on that. If there is an API for doing 
this, it's hidden well. :) So I fell back to just doing `if (Inconsistent)` 
instead, which has this quirk if the inconsistent version of the warning is 
disabled.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3064
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? 
diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
+  unsigned DiagID = isa(MD) ? DiagDtor : DiagFn;

dblaikie wrote:
> Generally I'd recommend default ref capture `[&]` on any lambda that doesn't 
> escape its scope - normal scopes don't need to document which variables you 
> use inside them, and I think the same applies to lambdas (bit more debatable 
> when the lambda is named and called later, even within the same scope - so if 
> you feel strongly about keeping it the way it is, that's OK)
I don't feel strongly at all; I'm fine with `[&]`. I'll make that change.



Comment at: clang/test/SemaCXX/warn-suggest-override:3-4
+
+class A {
+ public:
+  ~A() {}

dblaikie wrote:
> I'd probably simplify these tests by using struct, so everything's implicitly 
> public, rather than class and having to make things public.
> 
> Also probably member function declarations rather than definitions would be 
> simpler?
Can do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I'm generally on board with this, but would like @rsmith 's sign off to be sure.

I think it might be nice to make the -Wno-inconsistent-missing-override 
-Wsuggest-override situation a bit better (by having it still do the same thing 
as -Wsuggest-override) but I don't feel /too/ strongly about it.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3064
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? 
diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
+  unsigned DiagID = isa(MD) ? DiagDtor : DiagFn;

Generally I'd recommend default ref capture `[&]` on any lambda that doesn't 
escape its scope - normal scopes don't need to document which variables you use 
inside them, and I think the same applies to lambdas (bit more debatable when 
the lambda is named and called later, even within the same scope - so if you 
feel strongly about keeping it the way it is, that's OK)



Comment at: clang/test/SemaCXX/warn-suggest-override:3-4
+
+class A {
+ public:
+  ~A() {}

I'd probably simplify these tests by using struct, so everything's implicitly 
public, rather than class and having to make things public.

Also probably member function declarations rather than definitions would be 
simpler?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D82728#2136887 , @logan-5 wrote:

> In D82728#2135149 , @dblaikie wrote:
>
> > Is the implementation you're proposing fairly consistent with GCC's? Run it 
> > over any big codebases to check it warns in the same places GCC does?
>
>
> This patch has the same behavior as `-Wsuggest-override` in GCC >= 9. In GCC 
> <9, it would suggest adding `override` to `void foo() final`, but in GCC >=9, 
> `final` is enough to suppress the warning. This patch's `-Wsuggest-override`, 
> as well as Clang's pre-existing `-Winconsistent-missing-override`, are also 
> silenced by `final`. (https://godbolt.org/z/hbxLK6)
>
> I built Clang itself with a Clang that had this patch, and with GCC with 
> `-Wsuggest-override`, and compared the results--they were identical (except 
> for the warning text). (618 warnings, for those interested.)


Awesome! Really appreciate the data.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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

clang-formatted the diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+class A {
+ public:
+  ~A() {}
+  void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+  void run() {}
+};
+
+class C {
+ public:
+  virtual void run() {} // expected-note 2{{overridden virtual function is here}}
+  virtual ~C() {}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D() {}
+};
+
+class E : public C {
+ public:
+  virtual void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E() {}
+};
+
+class F : public C {
+ public:
+  void run() override {}
+  ~F() override {}
+};
+
+class G : public C {
+ public:
+  void run() final {}
+  ~G() final {}
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+class A {
+ public:
+  ~A() {}
+  virtual void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+};
+
+class C {
+ public:
+  virtual void run() {}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  void run() {}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,19 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
+  unsigned DiagID = isa(MD) ? DiagDtor : DiagFn;
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (Inconsistent)
+  EmitDiag(
+  diag::warn_inconsistent_destructor_marked_not_override_overriding,
+  diag::warn_inconsistent_function_marked_not_override_overriding);
+else
+  EmitDiag(diag::warn_suggest_destructor_marked_not_override_overriding,
+   diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
@@ -6749,13 +6756,10 @@
 }
   }
 
-  if (HasMethodWithOverrideControl &&
-  HasOverridingMethodWithoutOverrideControl) {
-// At least one method has the 'override' control declared.
-// Diagnose all other overridden methods which do not have 'override'
-// specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool HasInconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())
-  DiagnoseAbsenceOfOverrideControl(M);
+  DiagnoseAbsenceOfOverrideControl(M, HasInconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6961,7 +6961,7 @@
 
   /// DiagnoseAbsenceOfOverrideControl - Diagnose if 'override' keyword was
   /// not used in the dec

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

In D82728#2135149 , @dblaikie wrote:

> Is the implementation you're proposing fairly consistent with GCC's? Run it 
> over any big codebases to check it warns in the same places GCC does?


This patch has the same behavior as `-Wsuggest-override` in GCC >= 9. In GCC 
<9, it would suggest adding `override` to `void foo() final`, but in GCC >=9, 
`final` is enough to suppress the warning. This patch's `-Wsuggest-override`, 
as well as Clang's pre-existing `-Winconsistent-missing-override`, are also 
silenced by `final`. (https://godbolt.org/z/hbxLK6)

I built Clang itself with a Clang that had this patch, and with GCC with 
`-Wsuggest-override`, and compared the results--they were identical (except for 
the warning text). (618 warnings, for those interested.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D82728#2135142 , @logan-5 wrote:

> Glad this is generating some discussion. For my $0.02, I would also 
> (obviously) love to be able to enable this warning on all the codebases I 
> work on, and this patch was born out of a discussion on the C++ Slack with 
> another user who had found this warning very useful in GCC and was wondering 
> why Clang didn't have it yet.
>
> In D82728#2134072 , @dblaikie wrote:
>
> > The issue is that such a warning then needs to be off by default, because 
> > we can't assume the user's intent. And Clang's historically been fairly 
> > averse to off-by-default warnings due to low user-penetration (not zero, 
> > like I said, I imagine LLVM itself would use such a warning, were it 
> > implemented) & so concerns about the cost/benefit tradeoff of the added 
> > complexity (source code and runtime) of the feature.
>
>
> I agree `-Wsuggest-override` should be off by default, yet I suspect its 
> user-penetration will be much higher than other off-by-default warnings, due 
> to numerous instances of people on the Internet asking for 
>  this feature 
> , as well as the precedent for 
> it set by GCC.


Ah, I hadn't checked/wasn't mentioned whether GCC has this warning. If GCC 
already has it, there's usually an easy enough argument to be made for GCC 
compatibility being worthwhile that overcomes most of the objections unless the 
GCC warning is particularly problematic in some way that I doubt this is.

Is the implementation you're proposing fairly consistent with GCC's? Run it 
over any big codebases to check it warns in the same places GCC does?

> Moreover, since this implementation of this warning lies along the exact same 
> code paths as the already existing `-Winconsistent-missing-override`, the 
> added complexity from this patch is absolutely minimal.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

Glad this is generating some discussion. For my $0.02, I would also (obviously) 
love to be able to enable this warning on all the codebases I work on, and this 
patch was born out of a discussion on the C++ Slack with another user who had 
found this warning very useful in GCC and was wondering why Clang didn't have 
it yet.

In D82728#2134072 , @dblaikie wrote:

> The issue is that such a warning then needs to be off by default, because we 
> can't assume the user's intent. And Clang's historically been fairly averse 
> to off-by-default warnings due to low user-penetration (not zero, like I 
> said, I imagine LLVM itself would use such a warning, were it implemented) & 
> so concerns about the cost/benefit tradeoff of the added complexity (source 
> code and runtime) of the feature.


I agree `-Wsuggest-override` should be off by default, yet I suspect its 
user-penetration will be much higher than other off-by-default warnings, due to 
numerous instances of people on the Internet asking for 
 this feature 
, as well as the precedent for it 
set by GCC. Moreover, since this implementation of this warning lies along the 
exact same code paths as the already existing 
`-Winconsistent-missing-override`, the added complexity from this patch is 
absolutely minimal.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3075
+  : diag::
+warn_inconsistent_function_marked_not_override_overriding);
+else

Quuxplusone wrote:
> These linebreaks are super unfortunate. Could they be improved by doing it 
> like this?
> ```
> auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
>   unsigned DiagID = isa(MD) ? DiagDtor : DiagFn;
>   Diag(MD->getLocation(), DiagID) << MD->getDeclName();
>   const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
>   Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
> };
> if (Inconsistent)
>   
> EmitDiag(diag::warn_inconsistent_destructor_marked_not_override_overriding,
>
> diag::warn_inconsistent_function_marked_not_override_overriding);
> else
>   EmitDiag(diag::warn_suggest_destructor_marked_not_override_overriding
>diag::warn_suggest_function_marked_not_override_overriding);
> ```
Agreed. Good idea on the fix--needed one more line break (the first one still 
hit column 81), but it looks much better now.



Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:6
+  ~A() {}
+  void virtual run() {}
+};

Quuxplusone wrote:
> Surely this doesn't compile?!
Because of `void virtual`? It does, surprisingly, as it does in the test for 
warn-inconsistent-missing-destructor-override, where I pilfered this from.

Nevertheless, changed to `virtual void` for sanity's sake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

logan-5 updated this revision to Diff 275928.
logan-5 marked 6 inline comments as done.
logan-5 added a comment.

Addressed some feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+class A {
+ public:
+  ~A() {}
+  void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+  void run() {}
+};
+
+class C {
+ public:
+  virtual void run() {} // expected-note 2{{overridden virtual function is here}}
+  virtual ~C() {}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D() {}
+};
+
+class E : public C {
+ public:
+  virtual void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E() {}
+};
+
+class F : public C {
+ public:
+  void run() override {}
+  ~F() override {}
+};
+
+class G : public C {
+ public:
+  void run() final {}
+  ~G() final {}
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+class A {
+ public:
+  ~A() {}
+  virtual void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+};
+
+class C {
+ public:
+  virtual void run() {}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  void run() {}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,20 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
+  unsigned DiagID = isa(MD) ? DiagDtor : DiagFn;
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (Inconsistent)
+  EmitDiag(
+  diag::warn_inconsistent_destructor_marked_not_override_overriding,
+  diag::warn_inconsistent_function_marked_not_override_overriding);
+else
+  EmitDiag(
+  diag::warn_suggest_destructor_marked_not_override_overriding,
+  diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
@@ -6749,13 +6757,10 @@
 }
   }
 
-  if (HasMethodWithOverrideControl &&
-  HasOverridingMethodWithoutOverrideControl) {
-// At least one method has the 'override' control declared.
-// Diagnose all other overridden methods which do not have 'override'
-// specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool HasInconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())
-  DiagnoseAbsenceOfOverrideControl(M);
+  DiagnoseAbsenceOfOverrideControl(M, HasInconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6961,7 +6961,7 @@
 
   /// DiagnoseAbsenceOfOverrideControl - Diagnose if '

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D82728#2133951 , @Quuxplusone wrote:

> In D82728#2133720 , @dblaikie wrote:
>
> > (the presence of at least one "override" being a signal the user intended 
> > to use override and missed some [...]
>
>
> I'm in favor of `-Wsuggest-override`, and would definitely use it if it 
> existed.


I've no doubt a non-trivial number of folks would - we'd probably enable it in 
LLVM itself.

> The problem I see with `-Wmissing-override`, as it's been implemented, is 
> that it uses the wrong granularity for "intent": it looks only across the 
> methods of a single class, rather than across all the classes of a single 
> header, or across a single translation unit, or across my entire codebase. In 
> real life, I //always// want to look across my entire codebase (excluding 
> system headers). If //any// class in my project uses `override`, I want Clang 
> to take that as a clear declaration of intent to use `override` throughout; I 
> don't want Clang treating class A differently from class B. But of course 
> Clang can't look at my whole codebase simultaneously.

Right - Clang's doing its best (well, debatable - that's what we're debating) 
with what it's got.

> So the next best thing is to give the user a simple way to "preload the 
> intent flag": to say "As soon as you start processing //any// class, please 
> act as if intent has been declared for that class." Adding 
> `-Wsuggest-override` to my build line seems like a perfect way to implement 
> that "preload" facility.

The issue is that such a warning then needs to be off by default, because we 
can't assume the user's intent. And Clang's historically been fairly averse to 
off-by-default warnings due to low user-penetration (not zero, like I said, I 
imagine LLVM itself would use such a warning, were it implemented) & so 
concerns about the cost/benefit tradeoff of the added complexity (source code 
and runtime) of the feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

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

Quuxplusone added a comment.

In D82728#2133720 , @dblaikie wrote:

> (the presence of at least one "override" being a signal the user intended to 
> use override and missed some [...]


I'm in favor of `-Wsuggest-override`, and would definitely use it if it 
existed. The problem I see with `-Wmissing-override`, as it's been implemented, 
is that it uses the wrong granularity for "intent": it looks only across the 
methods of a single class, rather than across all the classes of a single 
header, or across a single translation unit, or across my entire codebase. In 
real life, I //always// want to look across my entire codebase (excluding 
system headers). If //any// class in my project uses `override`, I want Clang 
to take that as a clear declaration of intent to use `override` throughout; I 
don't want Clang treating class A differently from class B. But of course Clang 
can't look at my whole codebase simultaneously. So the next best thing is to 
give the user a simple way to "preload the intent flag": to say "As soon as you 
start processing //any// class, please act as if intent has been declared for 
that class." Adding `-Wsuggest-override` to my build line seems like a perfect 
way to implement that "preload" facility.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3075
+  : diag::
+warn_inconsistent_function_marked_not_override_overriding);
+else

These linebreaks are super unfortunate. Could they be improved by doing it like 
this?
```
auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
  unsigned DiagID = isa(MD) ? DiagDtor : DiagFn;
  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
};
if (Inconsistent)
  
EmitDiag(diag::warn_inconsistent_destructor_marked_not_override_overriding,
   diag::warn_inconsistent_function_marked_not_override_overriding);
else
  EmitDiag(diag::warn_suggest_destructor_marked_not_override_overriding
   diag::warn_suggest_function_marked_not_override_overriding);
```



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6764
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool InconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())

Can you s/InconsistentOverrideControl/HasInconsistentOverrideControl/ without 
causing bad linebreaks?



Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:6
+  ~A() {}
+  void virtual run() {}
+};

Surely this doesn't compile?!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Generally Clang tries to avoid stylistic warnings like this (& they are left to 
be addressed by tools like clang-tidy), hence why 
-Winconsistent-missing-override was implemented that way (the presence of at 
least one "override" being a signal the user intended to use override and 
missed some, rather than that maybe the user hadn't intended to use it at all 
(because they were compiling their code with multiple versions, etc)). (but I 
wouldn't outright rule this out - just mentioning the general principle here, 
perhaps @rsmith or others have other ideas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728



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

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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

Don't warn for destructors by default. This makes 
-Wsuggest-[destructor-]override more consistent with the behavior of 
-Winconsistent-missing-[destructor-]override, as well as gcc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+class A {
+ public:
+  ~A() {}
+  void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+  void run() {}
+};
+
+class C {
+ public:
+  virtual void run() {} // expected-note 2{{overridden virtual function is here}}
+  virtual ~C() {}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D() {}
+};
+
+class E : public C {
+ public:
+  virtual void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E() {}
+};
+
+class F : public C {
+ public:
+  void run() override {}
+  ~F() override {}
+};
+
+class G : public C {
+ public:
+  void run() final {}
+  ~G() final {}
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+class A {
+ public:
+  ~A() {}
+  void virtual run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+};
+
+class C {
+ public:
+  virtual void run() {}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  void run() {}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,23 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [this, MD](unsigned DiagID) {
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (Inconsistent)
+  EmitDiag(
+  isa(MD)
+  ? diag::
+warn_inconsistent_destructor_marked_not_override_overriding
+  : diag::
+warn_inconsistent_function_marked_not_override_overriding);
+else
+  EmitDiag(
+  isa(MD)
+  ? diag::warn_suggest_destructor_marked_not_override_overriding
+  : diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
@@ -6749,13 +6760,10 @@
 }
   }
 
-  if (HasMethodWithOverrideControl &&
-  HasOverridingMethodWithoutOverrideControl) {
-// At least one method has the 'override' control declared.
-// Diagnose all other overridden methods which do not have 'override'
-// specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool InconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())
-  DiagnoseAbsenceOfOverrideControl(M);
+  DiagnoseAbsenceOfOverrideControl(M, InconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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

Ran clang-format over the diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+class A {
+ public:
+  ~A() {}
+  void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+  void run() {}
+};
+
+class C {
+ public:
+  virtual void run() {} // expected-note 2{{overridden virtual function is here}}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  virtual void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
+
+class F : public C {
+ public:
+  void run() override {}
+  ~F() override {}
+};
+
+class G : public C {
+ public:
+  void run() final {}
+  ~G() final {}
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+class A {
+ public:
+  ~A() {}
+  void virtual run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+};
+
+class C {
+ public:
+  virtual void run() {}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  void run() {}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,23 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [this, MD](unsigned DiagID) {
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (Inconsistent)
+  EmitDiag(
+  isa(MD)
+  ? diag::
+warn_inconsistent_destructor_marked_not_override_overriding
+  : diag::
+warn_inconsistent_function_marked_not_override_overriding);
+else
+  EmitDiag(
+  isa(MD)
+  ? diag::warn_suggest_destructor_marked_not_override_overriding
+  : diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
@@ -6749,13 +6760,10 @@
 }
   }
 
-  if (HasMethodWithOverrideControl &&
-  HasOverridingMethodWithoutOverrideControl) {
-// At least one method has the 'override' control declared.
-// Diagnose all other overridden methods which do not have 'override'
-// specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+bool InconsistentOverrideControl = HasMethodWithOverrideControl;
 for (auto *M : Record->methods())
-  DiagnoseAbsenceOfOverrideControl(M);
+  DiagnoseAbsenceOfOverrideControl(M, InconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sem

[PATCH] D82728: [clang] Add -Wsuggest-override

[Logan Smith via Phabricator via cfe-commits]

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

This patch adds `-Wsuggest-override`, which allows for more aggressive 
enforcement of modern C++ best practices, as well as better compatibility with 
gcc, which has had its own `-Wsuggest-override` since version 5.1.

Clang already has `-Winconsistent-missing-override`, which only warns in the 
case where there is at least one function already marked `override` in a class. 
This warning strengthens that warning by suggesting the `override` keyword 
regardless of whether it is already present anywhere.

The text between suggest-override and inconsistent-missing-override is now 
shared, using `TextSubstitution` for the entire diagnostic text. I'm not sure 
if there is a better way to accomplish this.

This patch as written behaves strangely with 
`-Wno-inconsistent-missing-override -Wsuggest-override`. In that case, a 
warning is //only// generated if there are no overrides already marked (because 
inconsistent-missing-override 'wins' when choosing which diagnostic to emit). 
However, this is a fairly nonsensical use case, so in my opinion it's fine.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82728

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/warn-suggest-destructor-override
  clang/test/SemaCXX/warn-suggest-override

Index: clang/test/SemaCXX/warn-suggest-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+class A {
+ public:
+  ~A() {}
+  void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+  void run() {}
+};
+
+class C {
+ public:
+  virtual void run() {} // expected-note 2{{overridden virtual function is here}}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  virtual void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
+
+class F : public C {
+ public:
+  void run() override {}
+  ~F() override {}
+};
+
+class G : public C {
+ public:
+  void run() final {}
+  ~G() final {}
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+class A {
+ public:
+  ~A() {}
+  void virtual run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+};
+
+class C {
+ public:
+  virtual void run() {}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  void run() {}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
   << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr())
 return;
   CXXMethodDecl *MD = dyn_cast(D);
@@ -3061,12 +3061,19 @@
   return;
 
   if (MD->size_overridden_methods() > 0) {
-unsigned DiagID = isa(MD)
-  ? diag::warn_destructor_marked_not_override_overriding
-  : diag::warn_function_marked_not_override_overriding;
-Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+auto EmitDiag = [this, MD](unsigned DiagID) {
+  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+};
+if (Inconsistent)
+  EmitDiag(isa(MD)
+  ? diag::warn_inconsistent_destructor_marked_not_override_overriding
+