Index: clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt @@ -1,6 +1,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyPerformanceModule + ImplicitCastInLoopCheck.cpp PerformanceTidyModule.cpp UnnecessaryCopyInitialization.cpp Index: clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h +++ clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h @@ -0,0 +1,37 @@ +//===--- ImplicitCastInLoopCheck.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_PERFORMANCE_IMPLICIT_CAST_IN_LOOP_CHECK_H_ +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CAST_IN_LOOP_CHECK_H_ + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace performance { + +// 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 ImplicitCastInLoopCheck : public ClangTidyCheck { + public: + using ClangTidyCheck::ClangTidyCheck; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + + private: + void ReportAndFix(const ASTContext *Context, const VarDecl *VD, + const CXXOperatorCallExpr *OperatorCall); +}; + +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_IMPLICIT_CAST_IN_LOOP_CHECK_H_ Index: clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp @@ -0,0 +1,106 @@ +//===--- ImplicitCastInLoopCheck.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 "ImplicitCastInLoopCheck.h" + +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" + +namespace clang { + +using namespace ast_matchers; + +namespace tidy { +namespace performance { + +namespace { +// Checks if the stmt is a ImplicitCastExpr with a CastKind that is not a NoOp. +// The subtelty is that in some cases (user defined conversions), we can +// get to ImplicitCastExpr inside each other, with the outer one a NoOp. In this +// case we skip the first cast expr. +bool IsNonTrivialImplicitCast(const Stmt* ST) { + if (const auto* ICE = dyn_cast(ST)) { + return (ICE->getCastKind() != CK_NoOp) || + IsNonTrivialImplicitCast(ICE->getSubExpr()); + } + return false; +} +} // namespace + +void ImplicitCastInLoopCheck::registerMatchers( + ast_matchers::MatchFinder* Finder) { + // We look for const ref loop variables that (optionally inside an + // ExprWithCleanup) materialize a temporary, and contain a implicit cast. The + // check on the implicit cast is done in check() because we can't access + // implicit cast 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. + // Note that when the implicit cast is done through a user defined cast + // operator, the node is a CXXMemberCallExpr, not a CXXOperatorCallExpr, so + // it should not get caught by the cxxOperatorCallExpr() matcher. + Finder->addMatcher( + cxxForRangeStmt(hasLoopVariable( + varDecl(hasType(qualType(references(qualType(isConstQualified())))), + hasInitializer(expr(hasDescendant(cxxOperatorCallExpr().bind( + "operator-call"))) + .bind("init"))) + .bind("faulty-var"))), + this); +} + +void ImplicitCastInLoopCheck::check( + const ast_matchers::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"); + + if (const auto* Cleanup = dyn_cast(Init)) { + Init = Cleanup->getSubExpr(); + } + const auto* Materialized = dyn_cast(Init); + if (!Materialized) { + return; + } + + // We ignore NoOp casts. Those are generated if the * operator on the + // iterator returns a value instead of a reference, and the loop variable + // is a reference. This situation is fine (it probably produces the same + // code at the end). + if (IsNonTrivialImplicitCast(Materialized->getTemporary())) { + ReportAndFix(Result.Context, VD, OperatorCall); + } +} + +void ImplicitCastInLoopCheck::ReportAndFix( + const ASTContext *Context, const VarDecl *VD, + const CXXOperatorCallExpr *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 cast; you can either " + "change the type to the correct 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"; + PrintingPolicy Policy(Context->getLangOpts()); + Policy.SuppressTagKeyword = true; + + diag(VD->getLocStart(), Message) << VD->getName() + << ConstRefType.getAsString(Policy); +} + +} // namespace performance +} // namespace tidy +} // namespace clang Index: clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp +++ clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "ImplicitCastInLoopCheck.h" #include "UnnecessaryCopyInitialization.h" namespace clang { @@ -20,6 +21,8 @@ class PerformanceModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "performance-implicit-cast-in-loop"); CheckFactories.registerCheck( "performance-unnecessary-copy-initialization"); } Index: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst @@ -76,6 +76,7 @@ modernize-use-default modernize-use-nullptr modernize-use-override + performance-implicit-cast-in-loop readability-braces-around-statements readability-container-size-empty readability-else-after-return Index: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst @@ -0,0 +1,19 @@ +performance-implicit-cast-in-loop +================================= + +This warning appears in range-based loop with 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 cast has been added, which can for example result in +expensive deep copies. + +Example: + +.. code:: c++ + + map> my_map; + for (const pair>& p : my_map) {} + // The iterator type is in fact pair>, which means + // that the compiler added a cast, resulting in a copy of the vectors. + +The easiest solution is usually to use "const auto&" instead of writing the type +manually. Index: clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp +++ clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp @@ -0,0 +1,161 @@ +// RUN: %check_clang_tidy %s performance-implicit-cast-in-loop %t + +// ---------- Classes used in the tests ---------- + +// Iterator returning by value. +template +struct Iterator { + void operator++(); + T operator*(); + bool operator!=(const Iterator& other); +}; + +// Iterator returning by reference. +template +struct RefIterator { + void operator++(); + T& operator*(); + bool operator!=(const RefIterator& other); +}; + +// The template argument is an iterator type, and a view is an object you can +// run a for loop on. +template +struct View { + T begin(); + T end(); +}; + +// With this class, the implicit cast is a call to the (implicit) constructor of +// the class. +template +class ImplicitWrapper { + public: + // Implicit! + ImplicitWrapper(const T& t); +}; + +// With this class, the implicit cast is a call to the conversion operators of +// SimpleClass and ComplexClass. +template +class OperatorWrapper { + public: + explicit OperatorWrapper(const T& t); +}; + +struct SimpleClass { + int foo; + operator OperatorWrapper(); +}; + +// The materialize expression is not the same when the class has a destructor, +// so we make sure we cover that case too. +class ComplexClass { + public: + ComplexClass(); + ~ComplexClass(); + operator OperatorWrapper(); +}; + +typedef View> SimpleView; +typedef View> SimpleRefView; +typedef View> ComplexView; +typedef View> ComplexRefView; + +// ---------- The test themselves ---------- +// For each test we do, in the same order, const ref, non const ref, const +// value, non const value. + +void SimpleClassIterator() { + for (const SimpleClass& foo : SimpleView()) {} + // This line does not compile because a temporary cannot be assigned to a non + // const reference. + // for (SimpleClass& foo : SimpleView()) {} + for (const SimpleClass foo : SimpleView()) {} + for (SimpleClass foo : SimpleView()) {} +} + +void SimpleClassRefIterator() { + for (const SimpleClass& foo : SimpleRefView()) {} + for (SimpleClass& foo : SimpleRefView()) {} + for (const SimpleClass foo : SimpleRefView()) {} + for (SimpleClass foo : SimpleRefView()) {} +} + +void ComplexClassIterator() { + for (const ComplexClass& foo : ComplexView()) {} + // for (ComplexClass& foo : ComplexView()) {} + for (const ComplexClass foo : ComplexView()) {} + for (ComplexClass foo : ComplexView()) {} +} + +void ComplexClassRefIterator() { + for (const ComplexClass& foo : ComplexRefView()) {} + for (ComplexClass& foo : ComplexRefView()) {} + for (const ComplexClass foo : ComplexRefView()) {} + for (ComplexClass foo : ComplexRefView()) {} +} + +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 cast; you can either change the type to the correct 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-cast-in-loop] + // for (ImplicitWrapper& foo : SimpleView()) {} + for (const ImplicitWrapper foo : SimpleView()) {} + for (ImplicitWrapperfoo : SimpleView()) {} +} + +void ImplicitSimpleClassRefIterator() { + for (const ImplicitWrapper& foo : SimpleRefView()) {} + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const SimpleClass &'.*}} + // for (ImplicitWrapper& foo : SimpleRefView()) {} + for (const ImplicitWrapper foo : SimpleRefView()) {} + for (ImplicitWrapperfoo : SimpleRefView()) {} +} + +void ImplicitComplexClassIterator() { + for (const ImplicitWrapper& foo : ComplexView()) {} + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}} + // for (ImplicitWrapper& foo : ComplexView()) {} + for (const ImplicitWrapper foo : ComplexView()) {} + for (ImplicitWrapperfoo : ComplexView()) {} +} + +void ImplicitComplexClassRefIterator() { + for (const ImplicitWrapper& foo : ComplexRefView()) {} + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}} + // for (ImplicitWrapper& foo : ComplexRefView()) {} + for (const ImplicitWrapper foo : ComplexRefView()) {} + for (ImplicitWrapperfoo : ComplexRefView()) {} +} + +void OperatorSimpleClassIterator() { + for (const OperatorWrapper& foo : SimpleView()) {} + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const SimpleClass &'.*}} + // for (OperatorWrapper& foo : SimpleView()) {} + for (const OperatorWrapper foo : SimpleView()) {} + for (OperatorWrapperfoo : SimpleView()) {} +} + +void OperatorSimpleClassRefIterator() { + for (const OperatorWrapper& foo : SimpleRefView()) {} + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const SimpleClass &'.*}} + // for (OperatorWrapper& foo : SimpleRefView()) {} + for (const OperatorWrapper foo : SimpleRefView()) {} + for (OperatorWrapperfoo : SimpleRefView()) {} +} + +void OperatorComplexClassIterator() { + for (const OperatorWrapper& foo : ComplexView()) {} + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}} + // for (OperatorWrapper& foo : ComplexView()) {} + for (const OperatorWrapper foo : ComplexView()) {} + for (OperatorWrapperfoo : ComplexView()) {} +} + +void OperatorComplexClassRefIterator() { + for (const OperatorWrapper& foo : ComplexRefView()) {} + // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}} + // for (OperatorWrapper& foo : ComplexRefView()) {} + for (const OperatorWrapper foo : ComplexRefView()) {} + for (OperatorWrapperfoo : ComplexRefView()) {} +}