[PATCH] D137968: [clang-tidy] Ignore overriden methods in `readability-const-return-type`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added reviewers: njames93, thomasetter.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang-tools-extra.

Overrides are constrained by the signature of the overridden method, so a
warning on an override is frequently unactionable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137968

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/checkers/readability/const-return-type.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability/const-return-type.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability/const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/const-return-type.cpp
@@ -279,11 +279,11 @@
   // CHECK-NOT-FIXES: virtual int getC() = 0;
 };
 
-class PVDerive : public PVBase {
+class NVDerive : public PVBase {
 public:
-  const int getC() { return 1; }
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 
'const'-qualified at the top level, which may reduce code readability without 
improving const correctness
-  // CHECK-NOT-FIXES: int getC() { return 1; }
+  // Don't warn about overridden methods, because it may be impossible to make
+  // them non-const as the user may not be able to change the base class.
+  const int getC() override { return 1; }
 };
 
 // Don't warn about const auto types, because it may be impossible to make 
them non-const
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -166,6 +166,11 @@
   <clang-tidy/checks/readability/avoid-const-params-in-decls>` to not
   warn about `const` value parameters of declarations inside macros.
 
+- Change the behavior of :doc:`readability-const-return-type
+  <clang-tidy/checks/readability/const-return-type>` to not
+  warn about `const` return types in overridden functions since the derived
+  class cannot always choose to change the function signature.
+
 - Fixed crashes in :doc:`readability-braces-around-statements
   <clang-tidy/checks/readability/braces-around-statements>` and
   :doc:`readability-simplify-boolean-expr 
<clang-tidy/checks/readability/simplify-boolean-expr>`
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -114,7 +114,9 @@
   Finder->addMatcher(
       functionDecl(
           returns(allOf(isConstQualified(), unless(NonLocalConstType))),
-          anyOf(isDefinition(), cxxMethodDecl(isPure())))
+          anyOf(isDefinition(), cxxMethodDecl(isPure())),
+          // Overridden functions are not actionable.
+          unless(cxxMethodDecl(isOverride())))
           .bind("func"),
       this);
 }



Index: clang-tools-extra/test/clang-tidy/checkers/readability/const-return-type.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability/const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/const-return-type.cpp
@@ -279,11 +279,11 @@
   // CHECK-NOT-FIXES: virtual int getC() = 0;
 };
 
-class PVDerive : public PVBase {
+class NVDerive : public PVBase {
 public:
-  const int getC() { return 1; }
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
-  // CHECK-NOT-FIXES: int getC() { return 1; }
+  // Don't warn about overridden methods, because it may be impossible to make
+  // them non-const as the user may not be able to change the base class.
+  const int getC() override { return 1; }
 };
 
 // Don't warn about const auto types, because it may be impossible to make them non-const
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -166,6 +166,11 @@
   <clang-tidy/checks/readability/avoid-const-params-in-decls>` to not
   warn about `const` value parameters of declarations inside macros.
 
+- Change the behavior of :doc:`readability-const-return-type
+  <clang-tidy/checks/readability/const-return-type>` to not
+  warn about `const` return types in overridden functions since the derived
+  class cannot always choose to change the function signature.
+
 - Fixed crashes in :doc:`readability-braces-around-statements
   <clang-tidy/checks/readability/braces-around-statements>` and
   :doc:`readability-simplify-boolean-expr <clang-tidy/checks/readability/simplify-boolean-expr>`
Index: clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
@@ -114,7 +114,9 @@
   Finder->addMatcher(
       functionDecl(
           returns(allOf(isConstQualified(), unless(NonLocalConstType))),
-          anyOf(isDefinition(), cxxMethodDecl(isPure())))
+          anyOf(isDefinition(), cxxMethodDecl(isPure())),
+          // Overridden functions are not actionable.
+          unless(cxxMethodDecl(isOverride())))
           .bind("func"),
       this);
 }

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