[clang-tidy] Add check for function parameters that are const& to builtin types

[sdowney]

Summary

Add a check for const& to builtin types. Add FixIt replacing, for example, 'const& int i' with 'int i'.

[LegalizeAdulthood]

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

I think I would like the fixit to preserve the const-ness of the argument while still stripping it of it's reference.

[LegalizeAdulthood]

test/clang-tidy/misc-const-ref-builtin.cpp

33–35

I see tests here only for declarations, and not definitions of functions.

I also don't see any tests for:

• member functions
• template member functions
• template specializations
• tests where function signatures result from macro expansion, either in the body of a macro or as a macro argument

[etienneb]

drive-by, some comments.

clang-tidy/misc/ConstRefBuiltinCheck.cpp

52

Function is deferenced, but it's assumed possibly null at line 34 -> dyn_cast_or_null
If not, then you should use dyn_cast instead.

docs/clang-tidy/checks/misc-const-ref-builtin.rst

29

nit: instantion -> instantiation

[sdowney]

Add more test cases, covering templates, template specializations, function definitions, member function templates.

Check if parent function of the argument is a nullptr.

Fix spelling mistake.

[sdowney]

In D18191#376471, @LegalizeAdulthood wrote:

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

I think I would like the fixit to preserve the const-ness of the argument while still stripping it of it's reference.

The usual pattern for that is to have the definition use const, and the declaration not, since it's an implementation detail if the passed parameter is modified. And, unfortunately, I'm stuck with one compiler that mangles those two differently, causing link errors.

Having void foo(const int i) in a declaration makes me suspicious, since the const is meaningless.

I could make this an option? If the option is set, emit 'const type' if the Decl hasBody?

[sdowney]

Add tests with functions generated with MACROS

[sdowney]

Add tests for FixIts

[LegalizeAdulthood]

In D18191#377686, @sdowney wrote:

In D18191#376471, @LegalizeAdulthood wrote:

There is utility in the definition of a function in saying that an argument is const int i instead of int i. The const-ness declares the intent that this local variable is not going to be modified. However, there is that oddity in C++ that allows a declaration to say void f(int i); and the implementation to say void f(const int i) { ... }.

I think I would like the fixit to preserve the const-ness of the argument while still stripping it of it's reference.

The usual pattern for that is to have the definition use const, and the declaration not, since it's an implementation detail if the passed parameter is modified. And, unfortunately, I'm stuck with one compiler that mangles those two differently, causing link errors.

Having void foo(const int i) in a declaration makes me suspicious, since the const is meaningless.

I could make this an option? If the option is set, emit 'const type' if the Decl hasBody?

It seems reasonable to do this as a refinement after this check is in place.

[alexfh]

Adding Samuel, who's written a similar check internally and might want to upstream it or suggest improvements to this patch.

[sbenza]

As Alex mentioned, we have a test like this.
It also adds a hardcoded list of user-defined types that are known to be better when passed by value (eg. StringRef)

One big difference is that we decided to not trigger on typedefs.
We can't know that the typedef is documented to be trivial and it could change in the future.
The check actually verifies that the spelling is the expected spelling.
That skips things like macros, templates, type traits, typedefs, aliases, etc.

I could upstream that check and make the user-defined type list configurable.

[sdowney]

At least in my codebase, skipping templates is too strong. I run across ones where the const& parameter is not one controlled by a template. It's often a size_t.

I could easily see not fixing the typedef'd refs, also, although I think warning on them is still useful. Particularly if they can then be added to a list to be changed. E.g. size_t.

[sbenza]

In D18191#391168, @sdowney wrote:

At least in my codebase, skipping templates is too strong. I run across ones where the const& parameter is not one controlled by a template. It's often a size_t.

The only check we are doing (other than matching type) is matching spelling.
This means that it will only skip templates where that type is spelled, for example, using the template argument.

So it will skip this template <typename T> void Foo(const T&); but not this template <typename T> void Foo(const T&, const size_t&); (for the size_t argument).
It will also skip things like void Foo(const T&, const typename T::difference_type&);

The idea is that if the spelling is not exact, then there is some external factor that can change what the type means and make the by-value option less efficient.

I could easily see not fixing the typedef'd refs, also, although I think warning on them is still useful. Particularly if they can then be added to a list to be changed. E.g. size_t.

Warning on types that will never be added to the list and that should be taken by const& will lead to constant false positives.
We could make these warnings opt-in. This way some users might turn it on always or just turn it on to find the list of types.

[alexfh]

docs/clang-tidy/checks/misc-const-ref-builtin.rst

29

nit: enclose g with double backquotes, please. Same with other inline code snippets (int, double, etc.)

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
+  ConstRefBuiltinCheck.cpp
   DefinitionsInHeadersCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp

clang-tidy/misc/ConstRefBuiltinCheck.h

+//===--- ConstRefBuiltinCheck.h - clang-tidy---------------------*- C++ -*-===//
+//
+//                     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_MISC_CONST_REF_BUILTIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONST_REF_BUILTIN_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Checks for function parameters that are const& to builtin types.
+///
+/// Const reference to builtin type, such as int, adds an unnecessary layer of
+/// indirection. The Check will also warn on const& to typedef to builtin. It
+/// will not warn if the const& paramter was due to template instantiation.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-const-ref-builtin.html
+class ConstRefBuiltinCheck : public ClangTidyCheck {
+public:
+  ConstRefBuiltinCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONST_REF_BUILTIN_H

clang-tidy/misc/ConstRefBuiltinCheck.cpp

+//===--- ConstRefBuiltinCheck.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 "ConstRefBuiltinCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void ConstRefBuiltinCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      parmVarDecl(hasType(references(
+                      qualType(isConstQualified())
+                                         .bind("referenced_type"))),
+                  hasType(references(hasCanonicalType(builtinType()))),
+                  unless(hasType(references(substTemplateTypeParmType()))))
+          .bind("param"),
+      this);
+}
+
+void ConstRefBuiltinCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Parameter = Result.Nodes.getNodeAs<ParmVarDecl>("param");
+  const auto *Function =
+      dyn_cast_or_null<FunctionDecl>(Parameter->getParentFunctionOrMethod());
+
+  if (Function == nullptr)
+    return;
+
+  auto ReferencedType = *Result.Nodes.getNodeAs<QualType>("referenced_type");
+
+  auto ParamTL = Parameter->getTypeSourceInfo()->getTypeLoc();
+  auto TypeRange = CharSourceRange::getTokenRange(Parameter->getLocStart(),
+                                                  ParamTL.getLocEnd());
+
+  ReferencedType.removeLocalConst();
+
+  auto Fix =
+      FixItHint::CreateReplacement(TypeRange, ReferencedType.getAsString());
+
+  if (Parameter->getName().empty()) {
+    auto Diag = diag(Parameter->getLocation(),
+         "const reference to %0 type at index '%1' in function '%2'")
+      << ReferencedType
+      << Parameter->getFunctionScopeIndex()
+      << Function->getName()
+      << Fix;
+  } else {
+    auto Diag = diag(Parameter->getLocation(),
+         "const reference to %0 at parameter '%1' in function '%2'")
+      << ReferencedType
+      << Parameter->getName()
+      << Function->getName()
+      << Fix;
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
+#include "ConstRefBuiltinCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
     CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
         "misc-assign-operator-signature");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
+    CheckFactories.registerCheck<ConstRefBuiltinCheck>(
+        "misc-const-ref-builtin");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "misc-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectRoundings>(
         "misc-incorrect-roundings");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");

docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-assign-operator-signature
    misc-bool-pointer-implicit-conversion
+   misc-const-ref-builtin
    misc-definitions-in-headers
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-widening-cast
    misc-move-constructor-init

docs/clang-tidy/checks/misc-const-ref-builtin.rst

+.. title:: clang-tidy - misc-const-ref-builtin
+
+misc-const-ref-builtin
+======================
+
+This check will warn about function arguments that are const refs to builtin
+types, such as int or double. It will also warn about const refs to typedefs to
+builtin types. It will not flag function arguments that are due to template
+instantiations.
+
+Example code::
+
+  void f(const int& i);
+
+
+The parameter i will be flagged.
+
+Example code::
+
+  template<typename T>
+  void g(const T&);
+
+  void X() {
+      g(5);
+  }
+
+
+The instantiation of g will not be flagged.

test/clang-tidy/misc-const-ref-builtin.cpp

+// RUN: %check_clang_tidy %s misc-const-ref-builtin %t
+
+void f(const int& i);
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: const reference to 'int' at parameter 'i' in function 'f' [misc-const-ref-builtin]
+
+void f(const int& i) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: const reference to 'int' at parameter 'i' in function 'f' [misc-const-ref-builtin]
+  int k = i;
+}
+
+void f2(const int&);
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: const reference to 'int' type at index '0' in function 'f2' [misc-const-ref-builtin]
+
+void f2(const int& i) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: const reference to 'int' at parameter 'i' in function 'f2' [misc-const-ref-builtin]
+  int k = i;
+}
+
+typedef int Int;
+void t(const Int& i);
+// CHECK-MESSAGES: :[[@LINE-1]]:19:  warning: const reference to 'Int' (aka 'int') at parameter 'i' in function 't' [misc-const-ref-builtin]
+
+void t(const Int& i) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:19:  warning: const reference to 'Int' (aka 'int') at parameter 'i' in function 't' [misc-const-ref-builtin]
+  int k = i;
+}
+
+//Function template
+template<typename T>
+void g(const T& t_) {
+  T t = t_;
+}
+
+// Explicit Specialization
+template<>
+void g<long>(const long& l) {
+  //CHECK-MESSAGES: :[[@LINE-1]]:26: warning: const reference to 'long' at parameter 'l' in function 'g' [misc-const-ref-builtin]
+  long i = l;
+}
+
+
+// Explicit Specialization (deduced type)
+template<>
+void g(const double& d) {
+  //CHECK-MESSAGES: :[[@LINE-1]]:22:  warning: const reference to 'double' at parameter 'd' in function 'g' [misc-const-ref-builtin]
+ double f = d;
+}
+
+template<typename T>
+void g2(const T&, const char& c);
+//CHECK-MESSAGES: :[[@LINE-1]]:31: warning: const reference to 'char' at parameter 'c' in function 'g2' [misc-const-ref-builtin]
+
+template<typename T>
+class Klass {
+public:
+  void k(const T& t);
+  void k2(const short& s);
+  //CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const reference to 'short' at parameter 's' in function 'k2' [misc-const-ref-builtin]
+  void k3(const T& t, const short& s);
+  //CHECK-MESSAGES: :[[@LINE-1]]:36: warning: const reference to 'short' at parameter 's' in function 'k3' [misc-const-ref-builtin]
+};
+
+class K2 {
+public:
+  template<typename T>
+  void f(const T& t);
+};
+
+class Bar {};
+
+void X() {
+  g(5);
+  g(123.456);
+  g2(5,'a');
+  Klass<Bar> kbar;
+  K2 k2;
+  k2.f(6);
+};
+
+void f3();
+
+void h(int& i);
+
+void f4(const Bar& bar);
+
+class Baz {
+  int i;
+};
+
+void f5(const Baz& baz);
+
+void f(const int& i, const int& j);
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: const reference to 'int' at parameter 'i' in function 'f' [misc-const-ref-builtin]
+// CHECK-MESSAGES: :[[@LINE-2]]:33: warning: const reference to 'int' at parameter 'j' in function 'f' [misc-const-ref-builtin]