diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -6,7 +6,7 @@ add_clang_library(clangTidyPerformanceModule FasterStringFindCheck.cpp ForRangeCopyCheck.cpp - ImplicitConversionInLoopCheck.cpp + ImplicitConversionCheck.cpp InefficientAlgorithmCheck.cpp InefficientStringConcatenationCheck.cpp InefficientVectorOperationCheck.cpp diff --git a/clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h b/clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h rename from clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h rename to clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h --- a/clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h +++ b/clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h @@ -1,4 +1,4 @@ -//===--- ImplicitConversionInLoopCheck.h - clang-tidy------------*- C++ -*-===// +//===--- ImplicitConversionCheck.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. @@ -6,8 +6,8 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CONVERSION_IN_LOOP_CHECK_H_ -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CONVERSION_IN_LOOP_CHECK_H_ +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CONVERSION_CHECK_H_ +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CONVERSION_CHECK_H_ #include "../ClangTidyCheck.h" @@ -18,13 +18,13 @@ // Checks that in a for range loop, if the provided type is a reference, then // the underlying type is the one returned by the iterator (i.e. that there // isn't any implicit conversion). -class ImplicitConversionInLoopCheck : public ClangTidyCheck { +class ImplicitConversionCheck : public ClangTidyCheck { public: - ImplicitConversionInLoopCheck(StringRef Name, ClangTidyContext *Context) + ImplicitConversionCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context) {} - bool isLanguageVersionSupported(const LangOptions &LangOpts) const override{ - return LangOpts.CPlusPlus11; - } + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; @@ -37,4 +37,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CONVERSION_IN_LOOP_CHECK_H_ +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CONVERSION_CHECK_H_ diff --git a/clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp b/clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp rename from clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp rename to clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp --- a/clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp @@ -1,4 +1,4 @@ -//===--- ImplicitConversionInLoopCheck.cpp - clang-tidy--------------------===// +//===--- ImplicitConversionCheck.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. @@ -6,7 +6,7 @@ // //===----------------------------------------------------------------------===// -#include "ImplicitConversionInLoopCheck.h" +#include "ImplicitConversionCheck.h" #include "clang/AST/ASTContext.h" #include "clang/AST/Decl.h" @@ -27,20 +27,20 @@ static bool IsNonTrivialImplicitCast(const Stmt *ST) { if (const auto *ICE = dyn_cast(ST)) { return (ICE->getCastKind() != CK_NoOp) || - IsNonTrivialImplicitCast(ICE->getSubExpr()); + IsNonTrivialImplicitCast(ICE->getSubExpr()); } return false; } -void ImplicitConversionInLoopCheck::registerMatchers(MatchFinder *Finder) { - // We look for const ref loop variables that (optionally inside an - // ExprWithCleanup) materialize a temporary, and contain a implicit - // conversion. The check on the implicit conversion is done in check() because - // we can't access implicit conversion subnode via matchers: has() skips casts - // and materialize! We also bind on the call to operator* to get the proper - // type in the diagnostic message. We use both cxxOperatorCallExpr for user - // defined operator and unaryOperator when the iterator is a pointer, like - // for arrays or std::array. +void ImplicitConversionCheck::registerMatchers(MatchFinder *Finder) { + // We look for const ref variables that (optionally inside an ExprWithCleanup) + // materialize a temporary, and contain a implicit conversion. The check on + // the implicit conversion is done in check() because we can't access implicit + // conversion subnode via matchers: has() skips casts and materialize! We also + // bind on the call to operator* to get the proper type in the diagnostic + // message. We use both cxxOperatorCallExpr for user defined operator and + // unaryOperator when the iterator is a pointer, like for arrays or + // std::array. // // Note that when the implicit conversion is done through a user defined // conversion operator, the node is a CXXMemberCallExpr, not a @@ -49,26 +49,22 @@ Finder->addMatcher( traverse( ast_type_traits::TK_AsIs, - cxxForRangeStmt(hasLoopVariable( - varDecl( - hasType(qualType(references(qualType(isConstQualified())))), - hasInitializer( - expr(anyOf( - hasDescendant( - cxxOperatorCallExpr().bind("operator-call")), - hasDescendant(unaryOperator(hasOperatorName("*")) - .bind("operator-call")))) - .bind("init"))) - .bind("faulty-var")))), + varDecl( + hasType(qualType(references(qualType(isConstQualified())))), + hasInitializer( + expr(anyOf(hasDescendant( + cxxOperatorCallExpr().bind("operator-call")), + hasDescendant(unaryOperator(hasOperatorName("*")) + .bind("operator-call")))) + .bind("init"))) + .bind("faulty-var")), this); } -void ImplicitConversionInLoopCheck::check( - const MatchFinder::MatchResult &Result) { +void ImplicitConversionCheck::check(const MatchFinder::MatchResult &Result) { const auto *VD = Result.Nodes.getNodeAs("faulty-var"); const auto *Init = Result.Nodes.getNodeAs("init"); - const auto *OperatorCall = - Result.Nodes.getNodeAs("operator-call"); + const auto *OperatorCall = Result.Nodes.getNodeAs("operator-call"); if (const auto *Cleanup = dyn_cast(Init)) Init = Cleanup->getSubExpr(); @@ -85,18 +81,18 @@ ReportAndFix(Result.Context, VD, OperatorCall); } -void ImplicitConversionInLoopCheck::ReportAndFix( - const ASTContext *Context, const VarDecl *VD, - const Expr *OperatorCall) { +void ImplicitConversionCheck::ReportAndFix(const ASTContext *Context, + const VarDecl *VD, + const Expr *OperatorCall) { // We only match on const ref, so we should print a const ref version of the // type. QualType ConstType = OperatorCall->getType().withConst(); QualType ConstRefType = Context->getLValueReferenceType(ConstType); const char Message[] = - "the type of the loop variable %0 is different from the one returned " - "by the iterator and generates an implicit conversion; you can either " - "change the type to the matching one (%1 but 'const auto&' is always a " - "valid option) or remove the reference to make it explicit that you are " + "the type of the variable %0 is different from the one on the right hand " + "side: this generates an implicit conversion; you can either change the " + "type to the matching one (%1 but 'const auto&' is always a valid " + "option) or remove the reference to make it explicit that you are " "creating a new value"; diag(VD->getBeginLoc(), Message) << VD << ConstRefType; } diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -11,7 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "FasterStringFindCheck.h" #include "ForRangeCopyCheck.h" -#include "ImplicitConversionInLoopCheck.h" +#include "ImplicitConversionCheck.h" #include "InefficientAlgorithmCheck.h" #include "InefficientStringConcatenationCheck.h" #include "InefficientVectorOperationCheck.h" @@ -35,8 +35,8 @@ "performance-faster-string-find"); CheckFactories.registerCheck( "performance-for-range-copy"); - CheckFactories.registerCheck( - "performance-implicit-conversion-in-loop"); + CheckFactories.registerCheck( + "performance-implicit-conversion"); CheckFactories.registerCheck( "performance-inefficient-algorithm"); CheckFactories.registerCheck( 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 @@ -257,7 +257,7 @@ `openmp-use-default-none `_, `performance-faster-string-find `_, "Yes" `performance-for-range-copy `_, "Yes" - `performance-implicit-conversion-in-loop `_, + `performance-implicit-conversion `_, `performance-inefficient-algorithm `_, "Yes" `performance-inefficient-string-concatenation `_, `performance-inefficient-vector-operation `_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst --- a/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst @@ -2,11 +2,11 @@ .. title:: clang-tidy - performance-implicit-cast-in-loop .. meta:: - :http-equiv=refresh: 5;URL=performance-implicit-conversion-in-loop.html + :http-equiv=refresh: 5;URL=performance-implicit-conversion.html performance-implicit-cast-in-loop ================================= -This check has been renamed to `performance-implicit-conversion-in-loop -`_. +This check has been renamed to `performance-implicit-conversion +`_. diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst --- a/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst @@ -1,21 +1,12 @@ -.. title:: clang-tidy - performance-implicit-conversion-in-loop - -performance-implicit-conversion-in-loop -======================================= +:orphan: -This warning appears in a range-based loop with a loop variable of const ref -type where the type of the variable does not match the one returned by the -iterator. This means that an implicit conversion happens, which can for example -result in expensive deep copies. - -Example: +.. title:: clang-tidy - performance-implicit-conversion-in-loop +.. meta:: + :http-equiv=refresh: 5;URL=performance-implicit-conversion.html -.. code-block:: c++ +performance-implicit-cast-in-loop +================================= - map> my_map; - for (const pair>& p : my_map) {} - // The iterator type is in fact pair>, which means - // that the compiler added a conversion, resulting in a copy of the vectors. +This check has been renamed to `performance-implicit-conversion +`_. -The easiest solution is usually to use ``const auto&`` instead of writing the -type manually. diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst @@ -0,0 +1,32 @@ +.. title:: clang-tidy - performance-implicit-conversion + +performance-implicit-conversion +=============================== + +This warning appears a when a variable of const ref type binds to a temporary +that has a different type than the right hand side of the initialization. +In that case, an implicit conversion happens, which can for example +result in expensive deep copies. + +Example (loop variable): + +.. code-block:: c++ + + map> my_map; + for (const pair>& p : my_map) {} + +Example (block scope variable): + +.. code-block:: c++ + + map> my_map; + const pair>& p = *my_map.begin(); + +In both cases, the iterator type is in fact ``pair>``, +which means that the compiler added a conversion, resulting in a copy of the +vectors. + +The easiest solution is usually to use ``const auto&`` instead of writing the +type manually. +If the conversion if desired, avoid using lifetime extension to make it clear +that the intent is to create a new object (i.e., remove the ``&``). diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s performance-implicit-conversion-in-loop %t +// RUN: %check_clang_tidy %s performance-implicit-conversion %t // ---------- Classes used in the tests ---------- @@ -98,7 +98,7 @@ void ImplicitSimpleClassIterator() { for (const ImplicitWrapper& foo : SimpleView()) {} - // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the loop variable 'foo' is different from the one returned by the iterator and generates an implicit conversion; you can either change the type to the matching one ('const SimpleClass &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [performance-implicit-conversion-in-loop] + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the variable 'foo' is different from the one on the right hand side: this generates an implicit conversion; you can either change the type to the matching one ('const SimpleClass &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [performance-implicit-conversion] // for (ImplicitWrapper& foo : SimpleView()) {} for (const ImplicitWrapper foo : SimpleView()) {} for (ImplicitWrapper foo : SimpleView()) {} @@ -195,3 +195,9 @@ for (const OperatorWrapper foo : array) {} for (OperatorWrapper foo : array) {} } + +void NonLoopVariable() { + ComplexClass array[5]; + const OperatorWrapper &foo = *array; + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}} +} diff --git a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn --- a/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn +++ b/llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn @@ -14,7 +14,7 @@ sources = [ "FasterStringFindCheck.cpp", "ForRangeCopyCheck.cpp", - "ImplicitConversionInLoopCheck.cpp", + "ImplicitConversionCheck.cpp", "InefficientAlgorithmCheck.cpp", "InefficientStringConcatenationCheck.cpp", "InefficientVectorOperationCheck.cpp",