diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -11,6 +11,7 @@ NoRecursionCheck.cpp NonCopyableObjects.cpp NonPrivateMemberVariablesInClassesCheck.cpp + PodConstRefToValueCheck.cpp RedundantExpressionCheck.cpp StaticAssertCheck.cpp ThrowByValueCatchByReferenceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -15,6 +15,7 @@ #include "NoRecursionCheck.h" #include "NonCopyableObjects.h" #include "NonPrivateMemberVariablesInClassesCheck.h" +#include "PodConstRefToValueCheck.h" #include "RedundantExpressionCheck.h" #include "StaticAssertCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" @@ -41,6 +42,8 @@ "misc-non-copyable-objects"); CheckFactories.registerCheck( "misc-non-private-member-variables-in-classes"); + CheckFactories.registerCheck( + "misc-pod-const-ref-to-value"); CheckFactories.registerCheck( "misc-redundant-expression"); CheckFactories.registerCheck("misc-static-assert"); diff --git a/clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h b/clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.h @@ -0,0 +1,39 @@ +//===--- 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 diff --git a/clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp b/clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/PodConstRefToValueCheck.cpp @@ -0,0 +1,133 @@ +//===--- 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("func"); + const auto *ParamDecl = Result.Nodes.getNodeAs("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 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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -82,6 +82,12 @@ Reports identifiers whose names are too short. Currently checks local variables and function parameters only. +- New :doc:`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 ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -215,6 +215,7 @@ `misc-no-recursion `_, `misc-non-copyable-objects `_, `misc-non-private-member-variables-in-classes `_, + `misc-pod-const-ref-to-value `_, "Yes" `misc-redundant-expression `_, "Yes" `misc-static-assert `_, "Yes" `misc-throw-by-value-catch-by-reference `_, diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-pod-const-ref-to-value.rst @@ -0,0 +1,25 @@ +.. 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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc-pod-const-ref-to-value.cpp @@ -0,0 +1,75 @@ +// 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(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(d); +} + +class B { + static int g(double &d); +}; + +int B::g(double &d) { + return static_cast(d); +} +} // namespace n2 + +template +void f(Args &&...args); + +struct Widget { + int a[1000]; +}; +void f(const Widget &); + +template +struct Max { + static T max(const T &a, const T &b); +}; +int x = Max::max(1, 2); // passes `int` by const reference, but this is fine