courbet created this revision.
courbet added a reviewer: pilki.
Herald added subscribers: llvm-commits, xazax.hun, mgorny.
Herald added projects: clang, LLVM.
courbet requested review of this revision.
It turns out that there is no reason to restrict to loop variables. This
allows catching stuff like:
const std::pair<uint32, SomeLargeObject>& kv = *map.begin();
(did you notice the missing `const uint32_t`) ?
On our codebase, this roughly doubles the number of warnings compared to
the loop-only case.
The warnings mainly fall in two categories:
1. Lifetime extended temporary:
const std::function<...>& c = [](...) {};
Which is better written as one of:
const std::function<...> c = [](...) {}; // explicit
const auto c = [](...) {}; // no std::function
2. Copy + lifetime extension.
const std::pair<uint32, SomeLargeObject>& kv = *map.begin();
Better written as:
// no copy:
const std::pair<const uint32, SomeLargeObject>& kv = *map.begin();
const auto& kv = *map.begin();
// explicit
const std::pair<uint32, SomeLargeObject> kv = *map.begin();
// explicit, allows automatic move.
std::pair<uint32, SomeLargeObject> kv = *map.begin();
And rename the check.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D92179
Files:
clang-tools-extra/clang-tidy/performance/CMakeLists.txt
clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp
clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h
clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp
clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h
clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst
clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst
clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
===================================================================
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn
+++ 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",
Index: clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp
+++ 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<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 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<SimpleClass>& foo : SimpleView()) {}
for (const ImplicitWrapper<SimpleClass> foo : SimpleView()) {}
for (ImplicitWrapper<SimpleClass> foo : SimpleView()) {}
@@ -195,3 +195,9 @@
for (const OperatorWrapper<ComplexClass> foo : array) {}
for (OperatorWrapper<ComplexClass> foo : array) {}
}
+
+void NonLoopVariable() {
+ ComplexClass array[5];
+ const OperatorWrapper<ComplexClass> &foo = *array;
+ // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst
===================================================================
--- /dev/null
+++ 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<int, vector<string>> my_map;
+ for (const pair<int, vector<string>>& p : my_map) {}
+
+Example (block scope variable):
+
+.. code-block:: c++
+
+ map<int, vector<string>> my_map;
+ const pair<int, vector<string>>& p = *my_map.begin();
+
+In both cases, the iterator type is in fact ``pair<const int, vector<string>>``,
+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 ``&``).
Index: clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst
+++ 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<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 conversion, resulting in a copy of the vectors.
+This check has been renamed to `performance-implicit-conversion
+<performance-implicit-conversion.html>`_.
-The easiest solution is usually to use ``const auto&`` instead of writing the
-type manually.
Index: clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst
+++ 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
-<performance-implicit-conversion-in-loop.html>`_.
+This check has been renamed to `performance-implicit-conversion
+<performance-implicit-conversion.html>`_.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -257,7 +257,7 @@
`openmp-use-default-none <openmp-use-default-none.html>`_,
`performance-faster-string-find <performance-faster-string-find.html>`_, "Yes"
`performance-for-range-copy <performance-for-range-copy.html>`_, "Yes"
- `performance-implicit-conversion-in-loop <performance-implicit-conversion-in-loop.html>`_,
+ `performance-implicit-conversion <performance-implicit-conversion.html>`_,
`performance-inefficient-algorithm <performance-inefficient-algorithm.html>`_, "Yes"
`performance-inefficient-string-concatenation <performance-inefficient-string-concatenation.html>`_,
`performance-inefficient-vector-operation <performance-inefficient-vector-operation.html>`_, "Yes"
Index: clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ 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<ForRangeCopyCheck>(
"performance-for-range-copy");
- CheckFactories.registerCheck<ImplicitConversionInLoopCheck>(
- "performance-implicit-conversion-in-loop");
+ CheckFactories.registerCheck<ImplicitConversionCheck>(
+ "performance-implicit-conversion");
CheckFactories.registerCheck<InefficientAlgorithmCheck>(
"performance-inefficient-algorithm");
CheckFactories.registerCheck<InefficientStringConcatenationCheck>(
Index: clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h
+++ 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_
Index: clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp
+++ 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<ImplicitCastExpr>(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<VarDecl>("faulty-var");
const auto *Init = Result.Nodes.getNodeAs<Expr>("init");
- const auto *OperatorCall =
- Result.Nodes.getNodeAs<Expr>("operator-call");
+ const auto *OperatorCall = Result.Nodes.getNodeAs<Expr>("operator-call");
if (const auto *Cleanup = dyn_cast<ExprWithCleanups>(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;
}
Index: clang-tools-extra/clang-tidy/performance/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
