[clang-tidy] Catch more unwanted implicit conversions in performance-implicit-conversion-in-loop

[courbet]

Summary

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.

[Eugene.Zelenko]

Please mention renaming in Release Notes.

clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp

1

Please use space after clang-tidy.

clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h

1

Please fix length and use space after clang-tidy.

[courbet]

Add to release notes.

[courbet]

Thanks, comments addressed.

[aaron.ballman]

clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp

39

This check has been around since at least 2016 -- are we sure we want to rename it? @alexfh -- is this a case where we should leave the old check name as an alias?

Also, as we generalize the check, it starts to be a question of whether this should be in the frontend under one of the -Wconversion flags (or a new one).

clang-tools-extra/clang-tidy/performance/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyPerformanceModule
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
-  ImplicitConversionInLoopCheck.cpp
+  ImplicitConversionCheck.cpp
   InefficientAlgorithmCheck.cpp
   InefficientStringConcatenationCheck.cpp
   InefficientVectorOperationCheck.cpp
   MoveConstArgCheck.cpp
   MoveConstructorInitCheck.cpp
   NoAutomaticMoveCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   PerformanceTidyModule.cpp

clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.h

-//===--- 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.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#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"
 
 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 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 {
   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 Expr *OperatorCall);
 };
 
 } // namespace performance
 } // 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_

clang-tools-extra/clang-tidy/performance/ImplicitConversionCheck.cpp

-//===--- 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.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#include "ImplicitConversionInLoopCheck.h"
+#include "ImplicitConversionCheck.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"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 // 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.
 static bool IsNonTrivialImplicitCast(const Stmt *ST) {
   if (const auto *ICE = dyn_cast<ImplicitCastExpr>(ST)) {
     return (ICE->getCastKind() != CK_NoOp) ||
-            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
   // CXXOperatorCallExpr, so it should not get caught by the
   // cxxOperatorCallExpr() matcher.
   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")))),
+                  expr(anyOf(hasDescendant(
+              .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();
 
   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->getSubExpr()))
     ReportAndFix(Result.Context, VD, OperatorCall);
 }
 
-void ImplicitConversionInLoopCheck::ReportAndFix(
-    const ASTContext *Context, const VarDecl *VD,
+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;
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp

 //===-- PerformanceTidyModule.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.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "FasterStringFindCheck.h"
 #include "ForRangeCopyCheck.h"
-#include "ImplicitConversionInLoopCheck.h"
+#include "ImplicitConversionCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "InefficientStringConcatenationCheck.h"
 #include "InefficientVectorOperationCheck.h"
 #include "MoveConstArgCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NoAutomaticMoveCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "TriviallyDestructibleCheck.h"
 #include "TypePromotionInMathFnCheck.h"
 #include "UnnecessaryCopyInitialization.h"
 #include "UnnecessaryValueParamCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 class PerformanceModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FasterStringFindCheck>(
         "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>(
         "performance-inefficient-string-concatenation");
     CheckFactories.registerCheck<InefficientVectorOperationCheck>(
         "performance-inefficient-vector-operation");
     CheckFactories.registerCheck<MoveConstArgCheck>(
         "performance-move-const-arg");

clang-tools-extra/docs/ReleaseNotes.rst

 --------------------------
 
 - Checks that allow configuring names of headers to include now support wrapping
   the include in angle brackets to create a system include. For example,
   :doc:`cppcoreguidelines-init-variables
   <clang-tidy/checks/cppcoreguidelines-init-variables>` and
   :doc:`modernize-make-unique <clang-tidy/checks/modernize-make-unique>`.
 
+- :doc:`performance-implicit-conversion-in-loop
+  <clang-tidy/checks/performance-implicit-conversion-in-loop>` has been renamed
+  to :doc:`performance-implicit-conversion
+  <clang-tidy/checks/performance-implicit-conversion>`, and now warns all
+  variables instead of just loop variables.
+
 New modules
 ^^^^^^^^^^^
 
 - New ``altera`` module.
 
   Includes checks related to OpenCL for FPGA coding guidelines, based on the
   `Altera SDK for OpenCL: Best Practices Guide
   <https://www.altera.com/en_US/pdfs/literature/hb/opencl-sdk/aocl_optimization_guide.pdf>`_.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `objc-missing-hash <objc-missing-hash.html>`_,
    `objc-nsinvocation-argument-lifetime <objc-nsinvocation-argument-lifetime.html>`_, "Yes"
    `objc-property-declaration <objc-property-declaration.html>`_, "Yes"
    `objc-super-self <objc-super-self.html>`_, "Yes"
    `openmp-exception-escape <openmp-exception-escape.html>`_,
    `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"
    `performance-move-const-arg <performance-move-const-arg.html>`_, "Yes"
    `performance-move-constructor-init <performance-move-constructor-init.html>`_, "Yes"
    `performance-no-automatic-move <performance-no-automatic-move.html>`_,
    `performance-noexcept-move-constructor <performance-noexcept-move-constructor.html>`_, "Yes"
    `performance-trivially-destructible <performance-trivially-destructible.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/performance-implicit-cast-in-loop.rst

 :orphan:
 
 .. 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>`_.
 

clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion-in-loop.rst

-.. 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.

clang-tools-extra/docs/clang-tidy/checks/performance-implicit-conversion.rst

+.. 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 ``&``).

clang-tools-extra/test/clang-tidy/checkers/performance-implicit-conversion-in-loop.cpp

-// 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 ----------
 
 // Iterator returning by value.
 template <typename T>
 struct Iterator {
   void operator++();
   T operator*();
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 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()) {}
 }
 
 void ImplicitSimpleClassRefIterator() {
   for (const ImplicitWrapper<SimpleClass>& foo : SimpleRefView()) {}
   // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const SimpleClass &'.*}}

 void OperatorComplexClassArray() {
   ComplexClass array[5];
   for (const OperatorWrapper<ComplexClass>& foo : array) {}
   // CHECK-MESSAGES: [[@LINE-1]]:{{[0-9]*}}: warning: the type of the{{.*'const ComplexClass &'.*}}
   // for (OperatorWrapper<ComplexClass>& foo : array) {}
   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 &'.*}}
+}

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/performance/BUILD.gn

 static_library("performance") {
   output_name = "clangTidyPerformanceModule"
   configs += [ "//llvm/utils/gn/build:clang_code" ]
   deps = [
     "//clang-tools-extra/clang-tidy",
     "//clang-tools-extra/clang-tidy/utils",
     "//clang/lib/AST",
     "//clang/lib/ASTMatchers",
     "//clang/lib/Analysis",
     "//clang/lib/Basic",
     "//clang/lib/Lex",
     "//llvm/lib/Support",
   ]
   sources = [
     "FasterStringFindCheck.cpp",
     "ForRangeCopyCheck.cpp",
-    "ImplicitConversionInLoopCheck.cpp",
+    "ImplicitConversionCheck.cpp",
     "InefficientAlgorithmCheck.cpp",
     "InefficientStringConcatenationCheck.cpp",
     "InefficientVectorOperationCheck.cpp",
     "MoveConstArgCheck.cpp",
     "MoveConstructorInitCheck.cpp",
     "NoAutomaticMoveCheck.cpp",
     "NoexceptMoveConstructorCheck.cpp",
     "PerformanceTidyModule.cpp",
     "TriviallyDestructibleCheck.cpp",
     "TypePromotionInMathFnCheck.cpp",
     "UnnecessaryCopyInitialization.cpp",
     "UnnecessaryValueParamCheck.cpp",
   ]
 }