[clang-tools-extra] r259195 - [clang-tidy] Move implicit-cast-in-loop check to upstream.
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 29 07:21:34 PST 2016
Author: alexfh
Date: Fri Jan 29 09:21:32 2016
New Revision: 259195
URL: http://llvm.org/viewvc/llvm-project?rev=259195&view=rev
Log:
[clang-tidy] Move implicit-cast-in-loop check to upstream.
Summary: This is implemented originally by Alex Pilkiewicz (pilki at google.com).
Reviewers: alexfh
Subscribers: cfe-commits
Patch by Haojian Wu!
Differential Revision: http://reviews.llvm.org/D16721
Added:
clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp
clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst
clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
Modified: clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt?rev=259195&r1=259194&r2=259195&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/CMakeLists.txt Fri Jan 29 09:21:32 2016
@@ -1,6 +1,7 @@
set(LLVM_LINK_COMPONENTS support)
add_clang_library(clangTidyPerformanceModule
+ ImplicitCastInLoopCheck.cpp
PerformanceTidyModule.cpp
UnnecessaryCopyInitialization.cpp
Added: clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp?rev=259195&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.cpp Fri Jan 29 09:21:32 2016
@@ -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<ImplicitCastExpr>(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<VarDecl>("faulty-var");
+ const auto* Init = Result.Nodes.getNodeAs<Expr>("init");
+ const auto* OperatorCall =
+ Result.Nodes.getNodeAs<CXXOperatorCallExpr>("operator-call");
+
+ if (const auto* Cleanup = dyn_cast<ExprWithCleanups>(Init)) {
+ Init = Cleanup->getSubExpr();
+ }
+ const auto* Materialized = dyn_cast<MaterializeTemporaryExpr>(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
Added: clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h?rev=259195&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/performance/ImplicitCastInLoopCheck.h Fri Jan 29 09:21:32 2016
@@ -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_
Modified: clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp?rev=259195&r1=259194&r2=259195&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/PerformanceTidyModule.cpp Fri Jan 29 09:21:32 2016
@@ -11,6 +11,7 @@
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
+#include "ImplicitCastInLoopCheck.h"
#include "UnnecessaryCopyInitialization.h"
namespace clang {
@@ -20,6 +21,8 @@ namespace performance {
class PerformanceModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<ImplicitCastInLoopCheck>(
+ "performance-implicit-cast-in-loop");
CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
"performance-unnecessary-copy-initialization");
}
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=259195&r1=259194&r2=259195&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Fri Jan 29 09:21:32 2016
@@ -76,6 +76,7 @@ Clang-Tidy Checks
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
Added: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst?rev=259195&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst (added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst Fri Jan 29 09:21:32 2016
@@ -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<int, vector<string>> my_map;
+ for (const pair<int, vector<string>>& p : my_map) {}
+ // The iterator type is in fact pair<const int, vector<string>>, 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.
Added: clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp?rev=259195&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/performance_implicit_cast_in_loop.cpp Fri Jan 29 09:21:32 2016
@@ -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 <typename T>
+struct Iterator {
+ void operator++();
+ T operator*();
+ bool operator!=(const Iterator& other);
+};
+
+// Iterator returning by reference.
+template <typename T>
+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 <typename T>
+struct View {
+ T begin();
+ T end();
+};
+
+// With this class, the implicit cast is a call to the (implicit) constructor of
+// the class.
+template <typename T>
+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 <typename T>
+class OperatorWrapper {
+ public:
+ explicit OperatorWrapper(const T& t);
+};
+
+struct SimpleClass {
+ int foo;
+ operator OperatorWrapper<SimpleClass>();
+};
+
+// 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<ComplexClass>();
+};
+
+typedef View<Iterator<SimpleClass>> SimpleView;
+typedef View<RefIterator<SimpleClass>> SimpleRefView;
+typedef View<Iterator<ComplexClass>> ComplexView;
+typedef View<RefIterator<ComplexClass>> 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<SimpleClass>& 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<SimpleClass>& foo : SimpleView()) {}
+ for (const ImplicitWrapper<SimpleClass> foo : SimpleView()) {}
+ for (ImplicitWrapper<SimpleClass>foo : SimpleView()) {}
+}
+
+void ImplicitSimpleClassRefIterator() {
+ for (const ImplicitWrapper<SimpleClass>& foo : SimpleRefView()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const SimpleClass &'.*}}
+ // for (ImplicitWrapper<SimpleClass>& foo : SimpleRefView()) {}
+ for (const ImplicitWrapper<SimpleClass> foo : SimpleRefView()) {}
+ for (ImplicitWrapper<SimpleClass>foo : SimpleRefView()) {}
+}
+
+void ImplicitComplexClassIterator() {
+ for (const ImplicitWrapper<ComplexClass>& foo : ComplexView()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
+ // for (ImplicitWrapper<ComplexClass>& foo : ComplexView()) {}
+ for (const ImplicitWrapper<ComplexClass> foo : ComplexView()) {}
+ for (ImplicitWrapper<ComplexClass>foo : ComplexView()) {}
+}
+
+void ImplicitComplexClassRefIterator() {
+ for (const ImplicitWrapper<ComplexClass>& foo : ComplexRefView()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
+ // for (ImplicitWrapper<ComplexClass>& foo : ComplexRefView()) {}
+ for (const ImplicitWrapper<ComplexClass> foo : ComplexRefView()) {}
+ for (ImplicitWrapper<ComplexClass>foo : ComplexRefView()) {}
+}
+
+void OperatorSimpleClassIterator() {
+ for (const OperatorWrapper<SimpleClass>& foo : SimpleView()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const SimpleClass &'.*}}
+ // for (OperatorWrapper<SimpleClass>& foo : SimpleView()) {}
+ for (const OperatorWrapper<SimpleClass> foo : SimpleView()) {}
+ for (OperatorWrapper<SimpleClass>foo : SimpleView()) {}
+}
+
+void OperatorSimpleClassRefIterator() {
+ for (const OperatorWrapper<SimpleClass>& foo : SimpleRefView()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const SimpleClass &'.*}}
+ // for (OperatorWrapper<SimpleClass>& foo : SimpleRefView()) {}
+ for (const OperatorWrapper<SimpleClass> foo : SimpleRefView()) {}
+ for (OperatorWrapper<SimpleClass>foo : SimpleRefView()) {}
+}
+
+void OperatorComplexClassIterator() {
+ for (const OperatorWrapper<ComplexClass>& foo : ComplexView()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
+ // for (OperatorWrapper<ComplexClass>& foo : ComplexView()) {}
+ for (const OperatorWrapper<ComplexClass> foo : ComplexView()) {}
+ for (OperatorWrapper<ComplexClass>foo : ComplexView()) {}
+}
+
+void OperatorComplexClassRefIterator() {
+ for (const OperatorWrapper<ComplexClass>& foo : ComplexRefView()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
+ // for (OperatorWrapper<ComplexClass>& foo : ComplexRefView()) {}
+ for (const OperatorWrapper<ComplexClass> foo : ComplexRefView()) {}
+ for (OperatorWrapper<ComplexClass>foo : ComplexRefView()) {}
+}
More information about the cfe-commits
mailing list