baloghadamsoftware updated this revision to Diff 168274. baloghadamsoftware added a comment. Herald added a subscriber: mgorny.
Updated according to the comments. https://reviews.llvm.org/D52727 Files: clang-tidy/performance/ForRangeCopyCheck.cpp clang-tidy/performance/ForRangeCopyCheck.h clang-tidy/performance/UnnecessaryCopyInitialization.cpp clang-tidy/performance/UnnecessaryCopyInitialization.h clang-tidy/performance/UnnecessaryValueParamCheck.cpp clang-tidy/performance/UnnecessaryValueParamCheck.h clang-tidy/utils/CMakeLists.txt clang-tidy/utils/Matchers.cpp clang-tidy/utils/Matchers.h docs/clang-tidy/checks/performance-for-range-copy.rst docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst docs/clang-tidy/checks/performance-unnecessary-value-param.rst test/clang-tidy/performance-for-range-copy-allowed-types.cpp test/clang-tidy/performance-unnecessary-copy-initialization-allowed-types.cpp test/clang-tidy/performance-unnecessary-value-param-allowed-types.cpp
Index: test/clang-tidy/performance-unnecessary-value-param-allowed-types.cpp =================================================================== --- /dev/null +++ test/clang-tidy/performance-unnecessary-value-param-allowed-types.cpp @@ -0,0 +1,66 @@ +// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -config="{CheckOptions: [{key: performance-unnecessary-value-param.AllowedTypes, value: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$'}]}" -- + +struct SmartPointer { + ~SmartPointer(); +}; + +struct smart_pointer { + ~smart_pointer(); +}; + +struct SmartPtr { + ~SmartPtr(); +}; + +struct smart_ptr { + ~smart_ptr(); +}; + +struct SmartReference { + ~SmartReference(); +}; + +struct smart_reference { + ~smart_reference(); +}; + +struct SmartRef { + ~SmartRef(); +}; + +struct smart_ref { + ~smart_ref(); +}; + +struct OtherType { + ~OtherType(); +}; + +void negativeSmartPointer(SmartPointer P) { +} + +void negative_smart_pointer(smart_pointer p) { +} + +void negativeSmartPtr(SmartPtr P) { +} + +void negative_smart_ptr(smart_ptr p) { +} + +void negativeSmartReference(SmartReference R) { +} + +void negative_smart_reference(smart_reference r) { +} + +void negativeSmartRef(SmartRef R) { +} + +void negative_smart_ref(smart_ref r) { +} + +void positiveOtherType(OtherType O) { + // CHECK-MESSAGES: [[@LINE-1]]:34: warning: the parameter 'O' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param] + // CHECK-FIXES: void positiveOtherType(const OtherType& O) { +} Index: test/clang-tidy/performance-unnecessary-copy-initialization-allowed-types.cpp =================================================================== --- /dev/null +++ test/clang-tidy/performance-unnecessary-copy-initialization-allowed-types.cpp @@ -0,0 +1,85 @@ +// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t -- -config="{CheckOptions: [{key: performance-unnecessary-copy-initialization.AllowedTypes, value: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$'}]}" -- + +struct SmartPointer { + ~SmartPointer(); +}; + +struct smart_pointer { + ~smart_pointer(); +}; + +struct SmartPtr { + ~SmartPtr(); +}; + +struct smart_ptr { + ~smart_ptr(); +}; + +struct SmartReference { + ~SmartReference(); +}; + +struct smart_reference { + ~smart_reference(); +}; + +struct SmartRef { + ~SmartRef(); +}; + +struct smart_ref { + ~smart_ref(); +}; + +struct OtherType { + ~OtherType(); +}; + +const SmartPointer &getSmartPointer(); +const smart_pointer &get_smart_pointer(); +const SmartPtr &getSmartPtr(); +const smart_ptr &get_smart_ptr(); +const SmartReference &getSmartReference(); +const smart_reference &get_smart_reference(); +const SmartRef &getSmartRef(); +const smart_ref &get_smart_ref(); +const OtherType &getOtherType(); + +void negativeSmartPointer() { + const auto P = getSmartPointer(); +} + +void negative_smart_pointer() { + const auto p = get_smart_pointer(); +} + +void negativeSmartPtr() { + const auto P = getSmartPtr(); +} + +void negative_smart_ptr() { + const auto p = get_smart_ptr(); +} + +void negativeSmartReference() { + const auto R = getSmartReference(); +} + +void negative_smart_reference() { + const auto r = get_smart_reference(); +} + +void negativeSmartRef() { + const auto R = getSmartRef(); +} + +void negative_smart_ref() { + const auto r = get_smart_ref(); +} + +void positiveOtherType() { + const auto O = getOtherType(); + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'O' is copy-constructed from a const reference; consider making it a const reference [performance-unnecessary-copy-initialization] + // CHECK-FIXES: const auto& O = getOtherType(); +} Index: test/clang-tidy/performance-for-range-copy-allowed-types.cpp =================================================================== --- /dev/null +++ test/clang-tidy/performance-for-range-copy-allowed-types.cpp @@ -0,0 +1,112 @@ +// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -config="{CheckOptions: [{key: performance-for-range-copy.AllowedTypes, value: '[Pp]ointer$;[Pp]tr$;[Rr]ef(erence)?$'}]}" -- -std=c++11 -fno-delayed-template-parsing + +template <typename T> +struct Iterator { + void operator++() {} + const T& operator*() { + static T* TT = new T(); + return *TT; + } + bool operator!=(const Iterator &) { return false; } + typedef const T& const_reference; +}; +template <typename T> +struct View { + T begin() { return T(); } + T begin() const { return T(); } + T end() { return T(); } + T end() const { return T(); } + typedef typename T::const_reference const_reference; +}; + +struct SmartPointer { + ~SmartPointer(); +}; + +struct smart_pointer { + ~smart_pointer(); +}; + +struct SmartPtr { + ~SmartPtr(); +}; + +struct smart_ptr { + ~smart_ptr(); +}; + +struct SmartReference { + ~SmartReference(); +}; + +struct smart_reference { + ~smart_reference(); +}; + +struct SmartRef { + ~SmartRef(); +}; + +struct smart_ref { + ~smart_ref(); +}; + +struct OtherType { + ~OtherType(); +}; + +void negativeSmartPointer() { + for (auto P : View<Iterator<SmartPointer>>()) { + auto P2 = P; + } +} + +void negative_smart_pointer() { + for (auto p : View<Iterator<smart_pointer>>()) { + auto p2 = p; + } +} + +void negativeSmartPtr() { + for (auto P : View<Iterator<SmartPtr>>()) { + auto P2 = P; + } +} + +void negative_smart_ptr() { + for (auto p : View<Iterator<smart_ptr>>()) { + auto p2 = p; + } +} + +void negativeSmartReference() { + for (auto R : View<Iterator<SmartReference>>()) { + auto R2 = R; + } +} + +void negative_smart_reference() { + for (auto r : View<Iterator<smart_reference>>()) { + auto r2 = r; + } +} + +void negativeSmartRef() { + for (auto R : View<Iterator<SmartRef>>()) { + auto R2 = R; + } +} + +void negative_smart_ref() { + for (auto r : View<Iterator<smart_ref>>()) { + auto r2 = r; + } +} + +void positiveOtherType() { + for (auto O : View<Iterator<OtherType>>()) { + // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy] + // CHECK-FIXES: for (const auto& O : View<Iterator<OtherType>>()) { + auto O2 = O; + } +} Index: docs/clang-tidy/checks/performance-unnecessary-value-param.rst =================================================================== --- docs/clang-tidy/checks/performance-unnecessary-value-param.rst +++ docs/clang-tidy/checks/performance-unnecessary-value-param.rst @@ -61,3 +61,9 @@ A string specifying which include-style is used, `llvm` or `google`. Default is `llvm`. + +.. option:: AllowedTypes + + A semicolon-separated list of names of types allowed to be passed by value. + Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches every type + with suffix `Ref`, `ref`, `Reference` and `reference`. The default is empty. Index: docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst =================================================================== --- docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst +++ docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst @@ -35,3 +35,13 @@ string UnnecessaryCopy2 = UnnecessaryCopy1; UnnecessaryCopy2.find("bar"); } + +Options +------- + +.. option:: AllowedTypes + + A semicolon-separated list of names of types allowed to be initialized by + copying. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches + every type with suffix `Ref`, `ref`, `Reference` and `reference`. The + default is empty. Index: docs/clang-tidy/checks/performance-for-range-copy.rst =================================================================== --- docs/clang-tidy/checks/performance-for-range-copy.rst +++ docs/clang-tidy/checks/performance-for-range-copy.rst @@ -25,3 +25,10 @@ When non-zero, warns on any use of `auto` as the type of the range-based for loop variable. Default is `0`. + +.. option:: AllowedTypes + + A semicolon-separated list of names of types allowed to be copied in each + iteration. Regular expressions are accepted, e.g. `[Rr]ef(erence)?$` matches + every type with suffix `Ref`, `ref`, `Reference` and `reference`. The default + is empty. Index: clang-tidy/utils/Matchers.h =================================================================== --- clang-tidy/utils/Matchers.h +++ clang-tidy/utils/Matchers.h @@ -48,6 +48,11 @@ return referenceType(pointee(qualType(isConstQualified()))); } +using ast_matchers::internal::Matcher; + +Matcher<NamedDecl> +matchesAnyListedName(const std::vector<std::string> &NameList); + } // namespace matchers } // namespace tidy } // namespace clang Index: clang-tidy/utils/Matchers.cpp =================================================================== --- /dev/null +++ clang-tidy/utils/Matchers.cpp @@ -0,0 +1,34 @@ +//===--- Matchers.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 "Matchers.h" + +namespace clang { +namespace tidy { +namespace matchers { + +using ast_matchers::matchesName; + +Matcher<NamedDecl> +matchesAnyListedName(const std::vector<std::string> &NameList) { + SmallString<256> NameRegEx; + llvm::raw_svector_ostream NameOut(NameRegEx); + for (const std::string &Name : NameList) { + if (!NameRegEx.empty()) + NameOut << '|'; + NameOut << "(" << Name << ")"; + } + if (NameRegEx.empty()) + NameOut << "^(?!.*)"; + return matchesName(NameRegEx.str()); +} + +} // namespace matchers +} // namespace tidy +} // namespace clang Index: clang-tidy/utils/CMakeLists.txt =================================================================== --- clang-tidy/utils/CMakeLists.txt +++ clang-tidy/utils/CMakeLists.txt @@ -10,6 +10,7 @@ IncludeInserter.cpp IncludeSorter.cpp LexerUtils.cpp + Matchers.cpp NamespaceAliaser.cpp OptionsUtils.cpp TypeTraits.cpp Index: clang-tidy/performance/UnnecessaryValueParamCheck.h =================================================================== --- clang-tidy/performance/UnnecessaryValueParamCheck.h +++ clang-tidy/performance/UnnecessaryValueParamCheck.h @@ -40,6 +40,7 @@ MutationAnalyzers; std::unique_ptr<utils::IncludeInserter> Inserter; const utils::IncludeSorter::IncludeStyle IncludeStyle; + const std::vector<std::string> AllowedTypes; }; } // namespace performance Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp =================================================================== --- clang-tidy/performance/UnnecessaryValueParamCheck.cpp +++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp @@ -12,6 +12,7 @@ #include "../utils/DeclRefExprUtils.h" #include "../utils/FixItHintUtils.h" #include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" #include "../utils/TypeTraits.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Lex/Lexer.h" @@ -68,17 +69,22 @@ StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), IncludeStyle(utils::IncludeSorter::parseIncludeStyle( - Options.getLocalOrGlobal("IncludeStyle", "llvm"))) {} + Options.getLocalOrGlobal("IncludeStyle", "llvm"))), + AllowedTypes( + utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) { // This check is specific to C++ and doesn't apply to languages like // Objective-C. if (!getLangOpts().CPlusPlus) return; - const auto ExpensiveValueParamDecl = - parmVarDecl(hasType(hasCanonicalType(allOf( - unless(referenceType()), matchers::isExpensiveToCopy()))), - decl().bind("param")); + const auto ExpensiveValueParamDecl = parmVarDecl( + hasType(hasCanonicalType(allOf( + unless(anyOf(referenceType(), + hasDeclaration(namedDecl( + matchers::matchesAnyListedName(AllowedTypes))))), + matchers::isExpensiveToCopy()))), + decl().bind("param")); Finder->addMatcher( functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()), unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))), @@ -173,6 +179,8 @@ ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IncludeStyle", utils::IncludeSorter::toString(IncludeStyle)); + Options.store(Opts, "AllowedTypes", + utils::options::serializeStringList(AllowedTypes)); } void UnnecessaryValueParamCheck::onEndOfTranslationUnit() { Index: clang-tidy/performance/UnnecessaryCopyInitialization.h =================================================================== --- clang-tidy/performance/UnnecessaryCopyInitialization.h +++ clang-tidy/performance/UnnecessaryCopyInitialization.h @@ -26,18 +26,19 @@ // const references, and const pointers to const. class UnnecessaryCopyInitialization : public ClangTidyCheck { public: - UnnecessaryCopyInitialization(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + UnnecessaryCopyInitialization(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: void handleCopyFromMethodReturn(const VarDecl &Var, const Stmt &BlockStmt, bool IssueFix, const VarDecl *ObjectArg, ASTContext &Context); void handleCopyFromLocalVar(const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt, bool IssueFix, ASTContext &Context); + const std::vector<std::string> AllowedTypes; }; } // namespace performance Index: clang-tidy/performance/UnnecessaryCopyInitialization.cpp =================================================================== --- clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ clang-tidy/performance/UnnecessaryCopyInitialization.cpp @@ -12,6 +12,7 @@ #include "../utils/DeclRefExprUtils.h" #include "../utils/FixItHintUtils.h" #include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" namespace clang { namespace tidy { @@ -30,6 +31,12 @@ using namespace ::clang::ast_matchers; using utils::decl_ref_expr::isOnlyUsedAsConst; +UnnecessaryCopyInitialization::UnnecessaryCopyInitialization( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AllowedTypes( + utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} + void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) { auto ConstReference = referenceType(pointee(qualType(isConstQualified()))); auto ConstOrConstReference = @@ -50,12 +57,16 @@ callExpr(callee(functionDecl(returns(ConstReference))), unless(callee(cxxMethodDecl()))); - auto localVarCopiedFrom = [](const internal::Matcher<Expr> &CopyCtorArg) { + auto localVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) { return compoundStmt( forEachDescendant( declStmt( has(varDecl(hasLocalStorage(), - hasType(matchers::isExpensiveToCopy()), + hasType(hasCanonicalType( + allOf(matchers::isExpensiveToCopy(), + unless(hasDeclaration(namedDecl( + matchers::matchesAnyListedName( + AllowedTypes))))))), unless(isImplicit()), hasInitializer( cxxConstructExpr( @@ -84,6 +95,7 @@ const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>("objectArg"); const auto *BlockStmt = Result.Nodes.getNodeAs<Stmt>("blockStmt"); const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall"); + // Do not propose fixes if the DeclStmt has multiple VarDecls or in macros // since we cannot place them correctly. bool IssueFix = @@ -144,6 +156,12 @@ recordFixes(NewVar, Context, Diagnostic); } +void UnnecessaryCopyInitialization::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AllowedTypes", + utils::options::serializeStringList(AllowedTypes)); +} + } // namespace performance } // namespace tidy } // namespace clang Index: clang-tidy/performance/ForRangeCopyCheck.h =================================================================== --- clang-tidy/performance/ForRangeCopyCheck.h +++ clang-tidy/performance/ForRangeCopyCheck.h @@ -40,6 +40,7 @@ ASTContext &Context); const bool WarnOnAllAutoCopies; + const std::vector<std::string> AllowedTypes; }; } // namespace performance Index: clang-tidy/performance/ForRangeCopyCheck.cpp =================================================================== --- clang-tidy/performance/ForRangeCopyCheck.cpp +++ clang-tidy/performance/ForRangeCopyCheck.cpp @@ -10,6 +10,8 @@ #include "ForRangeCopyCheck.h" #include "../utils/DeclRefExprUtils.h" #include "../utils/FixItHintUtils.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" #include "../utils/TypeTraits.h" #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" @@ -21,26 +23,34 @@ ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), - WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)) {} + WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)), + AllowedTypes( + utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies); + Options.store(Opts, "AllowedTypes", + utils::options::serializeStringList(AllowedTypes)); } void ForRangeCopyCheck::registerMatchers(MatchFinder *Finder) { // Match loop variables that are not references or pointers or are already // initialized through MaterializeTemporaryExpr which indicates a type // conversion. auto LoopVar = varDecl( - hasType(hasCanonicalType(unless(anyOf(referenceType(), pointerType())))), + hasType(hasCanonicalType( + unless(anyOf(referenceType(), pointerType(), + hasDeclaration(namedDecl( + matchers::matchesAnyListedName(AllowedTypes))))))), unless(hasInitializer(expr(hasDescendant(materializeTemporaryExpr()))))); Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar"))) .bind("forRange"), this); } void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) { const auto *Var = Result.Nodes.getNodeAs<VarDecl>("loopVar"); + // Ignore code in macros since we can't place the fixes correctly. if (Var->getBeginLoc().isMacroID()) return;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits