[clang-tidy] Add a new clang-tidy check for trivially copyable types passed by const reference

[jmarrec]

Added content to the clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst file

[• Quuxplusone]

clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst

22

D107873 is related.

I'd like to see some tests/discussion around large types, e.g.

struct Widget { int a[1000]; };
void f(const Widget&);

(I think D107873 at least makes a conscious attempt to get the "would it be passed in registers?" check right.)

I'd like to see some tests/discussion around generic code, e.g.

template<class T>
struct Max {
    static T max(const T& a, const T& b);
};
int x = Max<int>::max(1, 2);  // passes `int` by const reference, but this is fine

[jmarrec]

clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp

4–6

This is now failing after I applied clang-format. int f1(const int& i) would correctly produce int f1(int i);, now it does int f1(inti);... I need to find a way to fix that.

[jmarrec]

clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst

22

Should I close this one in favor of https://reviews.llvm.org/D107873?

[jmarrec]

Added a MaxSize option. defaults to 16. Also skip templates. like in D107873

Dealt with the f(int &i) case instead of f(int& i)

[jmarrec]

Forgot to run clang format on the changes

[cjdb]

clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst

22

Up to you. There's always a chance that D107873 doesn't get approved. What I find most interesting is that we were independently working on this at the same time :-)

[jmarrec]

clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst

22

@cjdb I guess it's uncanny that we would both decide to do it at the same time. A bit unfortunate too probably, I could have saved a few hours :) but I learned something about clang-tidy so I don't mind at all!

Yours (D107873) is definitely better documented, you handle to opposite case (I only deal with const-ref -> value, not value -> const ret) and I just borrowed your implementation for MaxSize / ignoring shared_ptr

As far as I can tell from a cursory look, there are a couple of things I'm doing that you aren't:

• I am actually using Options, I don't think you are yet, even for MaxSize. I see from your documentation file that you plan to add a few
• The RestrictToBuiltInTypes I find an interesting option too
• I am removing the const as well in addition to the &. I guess you are just relying on using it in conjunction with readability-avoid-const-params-in-decls but I think that makes it harder to use (you can't just do -checks=-*,<yourcheck> you have to remember to add the readability-avoid-const-params-in-decls too...).

Perhaps we could discuss adding the above to your PR and consolidate over there to make a nice, feature-complete check?

[cjdb]

clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst

22

Sounds good.

[whisperity]

clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h

19–26

clang-tools-extra/docs/ReleaseNotes.rst

78

79

clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst

8

10

[jmarrec]

documentation updates

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyMiscModule
   DefinitionsInHeadersCheck.cpp
   MiscTidyModule.cpp
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoRecursionCheck.cpp
   NonCopyableObjects.cpp
   NonPrivateMemberVariablesInClassesCheck.cpp
+  PodConstRefToValueCheck.cpp
   RedundantExpressionCheck.cpp
   StaticAssertCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedUsingDeclsCheck.cpp

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoRecursionCheck.h"
 #include "NonCopyableObjects.h"
 #include "NonPrivateMemberVariablesInClassesCheck.h"
+#include "PodConstRefToValueCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedUsingDeclsCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<NonPrivateMemberVariablesInClassesCheck>(
         "misc-non-private-member-variables-in-classes");
+    CheckFactories.registerCheck<PodConstRefToValueCheck>(
+        "misc-pod-const-ref-to-value");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
         "misc-redundant-expression");
     CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
         "misc-throw-by-value-catch-by-reference");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(

clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h

+//===--- PodConstRefToValueCheck.h - clang-tidy -----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_PODCONSTREFTOVALUECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_PODCONSTREFTOVALUECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// The check detects when trivially_copyable types are passed by
+/// const-reference to a function and changes that to by value.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-pod-const-ref-to-value.html
+class PodConstRefToValueCheck : public ClangTidyCheck {
+public:
+  PodConstRefToValueCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  const bool RestrictToBuiltInTypes;
+  const int MaxSize = 16;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_PODCONSTREFTOVALUECHECK_H

clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp

+//===--- PodConstRefToValueCheck.cpp - clang-tidy -------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "PodConstRefToValueCheck.h"
+#include "../utils/LexerUtils.h"
+#include "../utils/OptionsUtils.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 misc {
+
+bool isSharedPtr(const QualType &T) {
+  if (auto *R = T->getAsCXXRecordDecl())
+    return R->getQualifiedNameAsString() == "std::shared_ptr" ||
+           R->getQualifiedNameAsString() == "boost::shared_ptr";
+  return false;
+}
+
+SourceRange getTypeRange(const ParmVarDecl &Param) {
+  return SourceRange(Param.getBeginLoc(),
+                     Param.getLocation().getLocWithOffset(-1));
+}
+
+PodConstRefToValueCheck::PodConstRefToValueCheck(StringRef Name,
+                                                 ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RestrictToBuiltInTypes(Options.get("RestrictToBuiltInTypes", false)),
+      MaxSize(16) {}
+
+void PodConstRefToValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "RestrictToBuiltInTypes", RestrictToBuiltInTypes);
+  Options.store(Opts, "MaxSize", MaxSize);
+}
+
+void PodConstRefToValueCheck::registerMatchers(MatchFinder *Finder) {
+  if (RestrictToBuiltInTypes) {
+    Finder->addMatcher(
+        functionDecl(
+            allOf(has(typeLoc(forEach(
+                      parmVarDecl(hasType(referenceType(pointee(qualType(
+                                      isConstQualified(), builtinType())))))
+                          .bind("param")))),
+                  unless(ast_matchers::isTemplateInstantiation())))
+            .bind("func"),
+        this);
+  } else {
+    Finder->addMatcher(
+        functionDecl(allOf(has(typeLoc(forEach(
+                               parmVarDecl(hasType(referenceType(pointee(
+                                               qualType(isConstQualified())))))
+                                   .bind("param")))),
+                           unless(ast_matchers::isTemplateInstantiation())))
+            .bind("func"),
+        this);
+  }
+}
+
+void PodConstRefToValueCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  const auto *ParamDecl = Result.Nodes.getNodeAs<ParmVarDecl>("param");
+
+  // if (!ParamDecl->getType().isLocalConstQualified()) {
+  //  return;
+  // }
+
+  const QualType &T = ParamDecl->getType().getNonReferenceType();
+
+  // If the parameter is NOT trivial to copy, const-ref makes sense
+  if (!T.isTriviallyCopyableType(*Result.Context)) {
+    return;
+  }
+
+  // Skip shared_ptr
+  if (isSharedPtr(T)) {
+    return;
+  }
+
+  // Skip types whose size is unknown or greater than MaxSize
+  Optional<CharUnits> Size = Result.Context->getTypeSizeInCharsIfKnown(T);
+  if (!Size.hasValue() || Size->getQuantity() > MaxSize) {
+    return;
+  }
+
+  auto Diag = diag(ParamDecl->getBeginLoc(),
+                   "argument %0 is a trivially copyable type and should not be "
+                   "passed by const-reference but by value");
+  if (ParamDecl->getName().empty()) {
+    for (unsigned int I = 0; I < FuncDecl->getNumParams(); ++I) {
+      if (ParamDecl == FuncDecl->getParamDecl(I)) {
+        Diag << (I + 1);
+        break;
+      }
+    }
+  } else {
+    Diag << ParamDecl;
+  }
+
+  // CharSourceRange FileRange = Lexer::makeFileCharRange(
+  // CharSourceRange::getTokenRange(getTypeRange(*ParamDecl)),
+  //*Result.SourceManager, getLangOpts());
+
+  // if (!FileRange.isValid()) {
+  // return;
+  //}
+
+  // auto Tok = tidy::utils::lexer::getQualifyingToken(
+  // tok::kw_const, FileRange, *Result.Context, *Result.SourceManager);
+  // if (!Tok)
+  // return;
+
+  Diag << FixItHint::CreateReplacement(
+      getTypeRange(*ParamDecl),
+      ParamDecl->getType()
+              .getNonReferenceType()
+              .getUnqualifiedType()
+              .getAsString(Result.Context->getPrintingPolicy()) +
+          " ");
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` check.
 
   Finds potentially incorrect calls to ``memcmp()`` based on properties of the arguments.
 
 - New :doc:`readability-identifier-length
   <clang-tidy/checks/readability-identifier-length>` check.
 
   Reports identifiers whose names are too short. Currently checks local variables and function parameters only.
 
+- New :doc:`misc-pod-const-ref-to-value
+  <clang-tidy/checks/misc-pod-const-ref-to-value>` check.
+
+  Finds when ``trivially_copyable`` types are passed by const-reference to a function
+  and changes that to by value.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-exp42-c
   <clang-tidy/checks/cert-exp42-c>` to
   :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `llvmlibc-implementation-in-namespace <llvmlibc-implementation-in-namespace.html>`_,
    `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
    `misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
    `misc-misplaced-const <misc-misplaced-const.html>`_,
    `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `misc-no-recursion <misc-no-recursion.html>`_,
    `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
    `misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_,
+   `misc-pod-const-ref-to-value <misc-pod-const-ref-to-value.html>`_, "Yes"
    `misc-redundant-expression <misc-redundant-expression.html>`_, "Yes"
    `misc-static-assert <misc-static-assert.html>`_, "Yes"
    `misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_,
    `misc-unconventional-assign-operator <misc-unconventional-assign-operator.html>`_,
    `misc-uniqueptr-reset-release <misc-uniqueptr-reset-release.html>`_, "Yes"
    `misc-unused-alias-decls <misc-unused-alias-decls.html>`_, "Yes"
    `misc-unused-parameters <misc-unused-parameters.html>`_, "Yes"
    `misc-unused-using-decls <misc-unused-using-decls.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst

+.. title:: clang-tidy - misc-pod-const-ref-to-value
+
+misc-pod-const-ref-to-value
+===========================
+
+This check detects when ``trivially_copyable`` types are passed by const-reference
+to a function and changes that to by value/
+
+For example in the following code, it is replaced by ``void f(int i, double d)``:
+
+.. code-block:: c++
+
+  void f(const int& i, const double& d);
+
+
+Options
+-------
+
+.. option:: RestrictToBuiltInTypes
+
+   If set to `true`, this check will limit itself to the `builtinType()` types. Default is `false`.
+
+.. option:: MaxSize
+
+   MaxSize: int, default 16. Above this size, passing by const-reference makes sense

clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp

+// RUN: %check_clang_tidy %s misc-pod-const-ref-to-value %t
+
+int f1(const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f1(int i);
+int f2(const int &i, double d);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f2(int i, double d);
+
+int f3(double d, const int &i);
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f3(double d, int i);
+
+int f4(const double &d, const double &d2);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-MESSAGES: :[[@LINE-2]]:25: warning: argument 'd2' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f4(double d, double d2);
+
+// clang-format off
+int f5(const int& i);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: argument 'i' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+// CHECK-FIXES: int f5(int i);
+// clang-format on
+
+namespace n1 {
+class A {
+  static int g(const double &d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: static int g(double d);
+};
+
+int A::g(const double &d) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: argument 'd' is a trivially copyable type and should not be passed by const-reference but by value [misc-pod-const-ref-to-value]
+  // CHECK-FIXES: int A::g(double d) {
+  return static_cast<int>(d);
+}
+} // namespace n1
+
+// Not triggering the check
+
+int f5(int i);
+int f6(int i, double d);
+int f7(int &i); // Might be used for return...
+
+namespace n2 {
+class A {
+  static int g(double d);
+};
+
+int A::g(double d) {
+  return static_cast<int>(d);
+}
+
+class B {
+  static int g(double &d);
+};
+
+int B::g(double &d) {
+  return static_cast<int>(d);
+}
+} // namespace n2
+
+template <typename... Args>
+void f(Args &&...args);
+
+struct Widget {
+  int a[1000];
+};
+void f(const Widget &);
+
+template <class T>
+struct Max {
+  static T max(const T &a, const T &b);
+};
+int x = Max<int>::max(1, 2); // passes `int` by const reference, but this is fine