[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Mitchell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdf7086fe: [clang-tidy] modernize-use-override new option 
AllowOverrideAndFinal (authored by mitchell-stellar).

Changed prior to commit:
  https://reviews.llvm.org/D70165?vs=229632=230040#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165

Files:
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-override.AllowOverrideAndFinal, value: 1}]}"
+
+struct Base {
+  virtual ~Base();
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void e();
+  virtual void f();
+  virtual void g();
+  virtual void h();
+  virtual void i();
+};
+
+struct Simple : public Base {
+  virtual ~Simple();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~Simple() override;
+  virtual void a() override;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() override;
+  virtual void b() final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() final;
+  virtual void c() final override;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void c() final override;
+  virtual void d() override final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void d() override final;
+  void e() final override;
+  void f() override final;
+  void g() final;
+  void h() override;
+  void i();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'override' or (rarely) 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void i() override;
+};
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
@@ -27,6 +27,14 @@
 
If set to non-zero, this check will not diagnose destructors. Default is `0`.
 
+.. option:: AllowOverrideAndFinal
+
+   If set to non-zero, this check will not diagnose ``override`` as redundant
+   with ``final``. This is useful when code will be compiled by a compiler with
+   warning/error checking flags requiring ``override`` explicitly on overriden
+   members, such as ``gcc -Wsuggest-override``/``gcc -Werror=suggest-override``.
+   Default is `0`.
+
 .. option:: OverrideSpelling
 
Specifies a macro to use instead of ``override``. This is useful when
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -170,6 +170,12 @@
   Finds non-static member functions that can be made ``const``
   because the functions don't use ``this`` in a non-const way.
 
+- Improved :doc:`modernize-use-override
+  ` check.
+
+  The check now supports the ``AllowOverrideAndFinal`` option to eliminate
+  conflicts with ``gcc -Wsuggest-override`` or ``gcc -Werror=suggest-override``.
+
 - The :doc:`readability-redundant-string-init
   ` check now supports a
   `StringNames` option enabling its application to custom string classes.
Index: clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
===
--- clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
+++ clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
@@ -26,6 +26,7 @@
 
 private:
   const bool IgnoreDestructors;
+  const bool AllowOverrideAndFinal;
   const std::string OverrideSpelling;
   const std::string FinalSpelling;
 };
Index: clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar reopened this revision.
mitchell-stellar added a comment.
This revision is now accepted and ready to land.

Reopening in order to correct the accidentally swapped commits between this 
ticket and https://reviews.llvm.org/D69238.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D70165#1751402 , @mitchell-stellar 
wrote:

> Oops, it looks like I mixed up this ticket with 
> https://reviews.llvm.org/D69238. Sorry about that. Would you like me to 
> revert both commits and then re-commit with the correct links, etc.?


No strong opinion, might be good to do (and reopen the differentials so they 
are closed with proper diffs)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar added a comment.

Oops, it looks like I mixed up this ticket with 
https://reviews.llvm.org/D69238. Sorry about that. Would you like me to revert 
both commits and then re-commit with the correct links, etc.?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D70165#1750606 , @mitchell-stellar 
wrote:

> Yes, there was a failing unit test that had to be fixed. I reverted the first 
> commit and then committed a version that fixed the failing test.


I mean, the commit message (including differential link) was wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar added a comment.

Yes, there was a failing unit test that had to be fixed. I reverted the first 
commit and then committed a version that fixed the failing test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added subscribers: mitchell-stellar, lebedev.ri.
lebedev.ri added a comment.

@poelmanc @mitchell-stellar
I'm confused by the diff - did this land? Was the correct patch committed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ah, so it *really* landed in rG06f3dabe4a2e85a32ade27c0769b6084c828a206 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Mitchell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG50e99563fb04: [clang-tidy] modernize-use-override new option 
AllowOverrideAndFinal (authored by mitchell-stellar).

Changed prior to commit:
  https://reviews.llvm.org/D70165?vs=229012=229632#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70165

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s 
readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
@@ -131,6 +130,11 @@
   // CHECK-FIXES: std::string a, b, c;
 
   std::string d = "u", e = "u", f = "u";
+
+  std::string g = "u", h = "", i = "uuu", j = "", k;
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: redundant string initialization
+  // CHECK-MESSAGES: [[@LINE-2]]:43: warning: redundant string initialization
+  // CHECK-FIXES: std::string g = "u", h, i = "uuu", j, k;
 }
 
 // These cases should not generate warnings.
Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init-msvc.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s 
readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
@@ -46,23 +46,23 @@
   // string bar("");
   Finder->addMatcher(
   namedDecl(
-  varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
-  
hasDeclaration(cxxRecordDecl(hasName("basic_string")),
-  hasInitializer(expr(ignoringImplicit(anyOf(
-  EmptyStringCtorExpr,
-  EmptyStringCtorExprWithTemporaries)))
- .bind("expr"))),
-  unless(parmVarDecl()))
-  .bind("decl"),
+  varDecl(
+  hasType(hasUnqualifiedDesugaredType(recordType(
+  hasDeclaration(cxxRecordDecl(hasName("basic_string")),
+  hasInitializer(expr(ignoringImplicit(anyOf(
+  EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)
+  .bind("vardecl"),
+  unless(parmVarDecl())),
   this);
 }
 
 void RedundantStringInitCheck::check(const MatchFinder::MatchResult ) {
-  const auto *CtorExpr = Result.Nodes.getNodeAs("expr");
-  const auto *Decl = Result.Nodes.getNodeAs("decl");
-  diag(CtorExpr->getExprLoc(), "redundant string initialization")
-  << FixItHint::CreateReplacement(CtorExpr->getSourceRange(),
-  Decl->getName());
+  const auto *VDecl = Result.Nodes.getNodeAs("vardecl");
+  // VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
+  // So start at getLocation() to span just 'foo = ""' or 'bar("")'.
+  SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
+  diag(VDecl->getLocation(), "redundant string initialization")
+  << FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
 }
 
 } // namespace readability


Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy %s readability-redundant-string-init %t
 
 namespace std {
 template 
@@ -131,6 +130,11 @@
   // CHECK-FIXES: std::string a, b, c;
 
   std::string 

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D70165#1745530 , @alexfh wrote:

> While I have no objections against this patch, I wonder whether someone had a 
> chance to ask GCC developers about this? Is it a conscious choice to suggest 
> `override` when `final` is present? What's the argument for doing so?


Thanks, someone should ask them as I believe this issue extends beyond 
clang-tidy: code with functions marked `final` //cannot// satisfy both `gcc 
-Wsuggest-override` and `clang -Winconsistent-missing-override`; `gcc` demands 
`override final` and `clang` demands just `final`.

Even if `clang` and `gcc` find a common ground, people will be stuck with 
current versions for quite a while, so this clang-tidy patch should prove 
helpful.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

While I have no objections against this patch, I wonder whether someone had a 
chance to ask GCC developers about this? Is it a conscious choice to suggest 
`override` when `final` is present? What's the argument for doing so?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D70165#1744007 , @JonasToth wrote:

> LGTM!
>  Did you check on a real code-base that suffers from the issue, if that works 
> as expected?


Thanks! I have now run it on our real code base and it worked as expected.

I lack commit access and would appreciate anyone committing this on my behalf.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Jonas Toth via Phabricator via cfe-commits]

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

LGTM!
Did you check on a real code-base that suffers from the issue, if that works as 
expected?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70165



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

[PATCH] D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc created this revision.
poelmanc added reviewers: alexfh, djasper.
Herald added subscribers: cfe-commits, mgehre.
Herald added a project: clang.

In addition to adding `override` wherever possible, clang-tidy's 
`modernize-use-override` nicely removes `virtual` when `override` or `final` is 
specified, and further removes `override` when `final` is specified. While this 
is great default behavior, when code needs to be compiled with `gcc` at high 
warning levels that include `gcc -Wsuggest-override` or `gcc 
-Werror=suggest-override`, clang-tidy's removal of the redundant `override` 
keyword causes gcc to emit a warning or error. This discrepancy / conflict has 
been noted by others including a comment on Stack Overflow 

 and by Mozzilla's Firefox developers 
.

This patch adds an AllowOverrideAndFinal option defaulting to `0` - thus 
preserving current behavior - that when enabled allows both `override` and 
`final` to co-exist, while still fixing all other issues.

The patch includes a test file verifying all combinations of 
``virtual``/``override``/``final``, and mentions the new option in the release 
notes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D70165

Files:
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-override.AllowOverrideAndFinal, value: 1}]}"
+
+struct Base {
+  virtual ~Base();
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void e();
+  virtual void f();
+  virtual void g();
+  virtual void h();
+  virtual void i();
+};
+
+struct Simple : public Base {
+  virtual ~Simple();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~Simple() override;
+  virtual void a() override;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() override;
+  virtual void b() final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() final;
+  virtual void c() final override;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void c() final override;
+  virtual void d() override final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void d() override final;
+  void e() final override;
+  void f() override final;
+  void g() final;
+  void h() override;
+  void i();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'override' or (rarely) 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void i() override;
+};
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst
@@ -27,6 +27,14 @@
 
If set to non-zero, this check will not diagnose destructors. Default is `0`.
 
+.. option:: AllowOverrideAndFinal
+
+   If set to non-zero, this check will not diagnose ``override`` as redundant
+   with ``final``. This is useful when code will be compiled by a compiler with
+   warning/error checking flags requiring ``override`` explicitly on overriden
+   members, such as ``gcc -Wsuggest-override``/``gcc -Werror=suggest-override``.
+   Default is `0`.
+
 .. option:: OverrideSpelling
 
Specifies a macro to use instead of ``override``. This is useful when
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -153,6 +153,12 @@
   Finds non-static member functions that can be made ``const``