readability check for const params in declarations

[fowles]

Summary

Adds a clang-tidy warning for top-level consts in function declarations.

[alexfh]

Looks good in general. A few nits.

clang-tidy/readability/AvoidConstParamsInDecls.cpp

23

"Function names ... should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())."
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

51

nit: The Diag name is more common for this purpose.

52–54

This is the best wording I could come up with in an internal review, but it's still not ideal. If someone knows how to improve this, ideas are welcome.

docs/clang-tidy/checks/readability-avoid-const-params-in-decls.rst

5

To many '='s.

9

Either "do not have any effect on ..." or "do not affect ..."

16

Please use proper capitalization and punctuation in the comments.

[fowles]

• review comments

[fowles]

ping?

[alexfh]

Looks good with one nit. Thank you for the new check!

Do you need me to submit the patch for you?

clang-tidy/readability/AvoidConstParamsInDecls.cpp

24

I suspect this will fail to build in MSVC 2013, since it doesn't support uniform initialization. But it might be just initializer lists that it doesn't support. We can try.

32

"Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

[fowles]

• review comments
• rename variables and remove MSVC compat issues

[alexfh]

Actually, we missed one thing: may I ask you to update docs/ReleaseNotes.rst with a short description of the new check?

[fowles]

My attempts to do this end with:

$ arc diff
Linting...
No lint engine configured for this project.
Running unit tests...
No unit test engine is configured for this project.
SKIP STAGING Unable to determine repository for this change.
Exception
ERR_CLOSED: This revision has already been closed.
(Run with --trace for a full exception trace.)

[LegalizeAdulthood]

You can submit the release notes changes in a new patch.

[fowles]

that is what I was trying to do. I can't seem to make arc play nice.

[LegalizeAdulthood]

Maybe you need to rebase first? I haven't used arc.

[fowles]

Helped by sbenza@ http://reviews.llvm.org/D18608

clang-tidy/readability/AvoidConstParamsInDecls.h

+//===--- AvoidConstParamsInDecls.h - clang-tidy----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOID_CONST_PARAMS_IN_DECLS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOID_CONST_PARAMS_IN_DECLS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+// Detect function declarations that have const value parameters and discourage
+// them.
+class AvoidConstParamsInDecls : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOID_CONST_PARAMS_IN_DECLS_H

clang-tidy/readability/AvoidConstParamsInDecls.cpp

+//===--- AvoidConstParamsInDecls.cpp - clang-tidy--------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidConstParamsInDecls.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+namespace {
+
+SourceRange getTypeRange(const ParmVarDecl &Param) {
+  if (Param.getIdentifier() != nullptr)
+    return {Param.getLocStart(), Param.getLocEnd().getLocWithOffset(-1)};
+  return Param.getSourceRange();
+}
+
+} // namespace
+
+
+void AvoidConstParamsInDecls::registerMatchers(MatchFinder *Finder) {
+  const auto const_param_decl =
+      parmVarDecl(hasType(qualType(isConstQualified()))).bind("param");
+  Finder->addMatcher(functionDecl(unless(isDefinition()),
+                                  has(typeLoc(forEach(const_param_decl))))
+                         .bind("func"),
+                     this);
+}
+
+void AvoidConstParamsInDecls::check(const MatchFinder::MatchResult &Result) {
+  const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
+
+  QualType Type = Param->getType();
+  if (!Type.isLocalConstQualified())
+    return;
+
+  Type.removeLocalConst();
+
+  auto Diag = diag(Param->getLocStart(),
+                   "parameter %0 is const-qualified in the function "
+                   "declaration; const-qualification of parameters only has an "
+                   "effect in function definitions");
+  if (Param->getName().empty()) {
+    for (unsigned int i = 0; i < Func->getNumParams(); ++i) {
+      if (Param == Func->getParamDecl(i)) {
+        Diag << (i + 1);
+        break;
+      }
+    }
+  } else {
+    Diag << Param;
+  }
+  Diag << FixItHint::CreateReplacement(getTypeRange(*Param),
+                                       Type.getAsString());
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
+  AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp

clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AvoidConstParamsInDecls>(
+        "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-default
    modernize-use-nullptr
    modernize-use-override
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop
+   readability-avoid-const-params-in-decls
    readability-braces-around-statements
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-control-flow
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-uniqueptr-delete-release

docs/clang-tidy/checks/readability-avoid-const-params-in-decls.rst

+.. title:: clang-tidy - readability-avoid-const-params-in-decls
+
+readability-avoid-const-params-in-decls
+=======================================
+
+Checks whether a function declaration has parameters that are top level const.
+
+`const` values in declarations do not affect the signature of a function, so
+they should not be put there.  For example:
+
+Examples:
+
+.. code:: c++
+
+  void f(const string);   // Bad: const is top level.
+  void f(const string&);  // Good: const is not top level.
+

test/clang-tidy/readability-avoid-const-params-in-decls.cpp

+// RUN: %check_clang_tidy %s readability-avoid-const-params-in-decls %t
+
+using alias_type = bool;
+using alias_const_type = const bool;
+
+
+void F1(const int i);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'i' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]
+// CHECK-FIXES: void F1(int i);
+
+void F2(const int *const i);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'i' is const-qualified
+// CHECK-FIXES: void F2(const int * i);
+
+void F3(int const i);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'i' is const-qualified
+// CHECK-FIXES: void F3(int i);
+
+void F4(alias_type const i);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'i' is const-qualified
+// CHECK-FIXES: void F4(alias_type i);
+
+void F5(const int);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 1 is const-qualified
+// CHECK-FIXES: void F5(int);
+
+void F6(const int *const);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 1 is const-qualified
+// BUG(b/27584482): void F6(const int *);  should be produced
+
+void F7(int, const int);
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: parameter 2 is const-qualified
+// CHECK-FIXES: void F7(int, int);
+
+void F8(const int, const int);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 1 is const-qualified
+// CHECK-MESSAGES: :[[@LINE-2]]:20: warning: parameter 2 is const-qualified
+// CHECK-FIXES: void F8(int, int);
+
+void F9(const int long_name);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter 'long_name'
+// CHECK-FIXES: void F9(int long_name);
+
+void F10(const int *const *const long_name);
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: parameter 'long_name'
+// CHECK-FIXES: void F10(const int *const * long_name);
+
+
+struct Foo {
+  Foo(const int i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: parameter 'i'
+  // CHECK-FIXES: Foo(int i);
+
+  void operator()(const int i);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: parameter 'i'
+  // CHECK-FIXES: void operator()(int i);
+};
+
+// Do not match on definitions
+void NF1(const int i) {}
+void NF2(const int *const i) {}
+void NF3(int const i) {}
+void NF4(alias_type const i) {}
+void NF5(const int) {}
+void NF6(const int *const) {}
+void NF7(int, const int) {}
+void NF8(const int, const int) {}
+
+// Do not match on other stuff
+void NF(const alias_type& i);
+void NF(const int &i);
+void NF(const int *i);
+void NF(alias_const_type i);
+void NF(const alias_type&);
+void NF(const int&);
+void NF(const int*);
+void NF(const int* const*);
+void NF(alias_const_type);