[PATCH] D78223: [clang-tidy] performance-range-for-copy only for copy.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF created this revision.
EricWF added reviewers: fowles, shuaiwang.
EricWF added a project: clang-tools-extra.
Herald added subscribers: mgehre, xazax.hun.

Currently the range-for-copy incorrectly suggests changing a

  by-value loop var to a reference to avoid copies even when:
  
  (1) A converting constructor was used. Or,
  (2) The argument to the copy constructor is not an lvalue.
  
  For example:
  
  for (const std::string sv : std::vector<const char*>{}) {
    // warning:
    //  the loop variable's type is not a reference type;
    //  this creates a copy in each iteration;
    //  consider making this a reference
  }
  
  In these cases we can't actually avoid the copy, and reference
  binding + lifetime extending a temporary is not a "fix" we should
  be suggesting.
  
  Admittedly, cases like the example should be fixed by the clang-tidy
  user, but at minimum we need to be clearer about when a copy is made
  and when a user defined conversion occurs (and that conversion may
  be semantically important).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D78223

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
@@ -1,4 +1,5 @@
 // RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s -std=c++17 performance-for-range-copy %t -- -- -fno-delayed-template-parsing
 
 namespace std {
 
@@ -46,6 +47,7 @@
   S &operator=(const S &);
 };
 
+
 struct Convertible {
   operator S() const {
     return S();
@@ -61,12 +63,16 @@
   Convertible C[0];
   for (const S &S1 : C) {
   }
+  for (const S S2 : C) {
+  }
 }
 
 void negativeImplicitConstructorConversion() {
   ConstructorConvertible C[0];
   for (const S &S1 : C) {
   }
+  for (const S S2 : C) {
+  }
 }
 
 template <typename T>
Index: clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -60,6 +60,18 @@
   handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context);
 }
 
+// Returns true iff the specified variable is initialized via copy construction
+// and the argument to the copy constructor is an lvalue.
+//
+// Note: We don't want to suggest reference binding + lifetime extending a
+// loop index because it's less clear than copying the temporary.
+static bool isCopyConstructedFromLValueArg(const VarDecl &LoopVar, ASTContext& Ctx) {
+  const auto *Init = dyn_cast_or_null<CXXConstructExpr>(LoopVar.getInit());
+  if (!Init || !Init->getConstructor()->isCopyConstructor() || Init->getNumArgs() < 1)
+    return false;
+  return Init->getArg(0)->isLValue();
+}
+
 bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
                                              ASTContext &Context) {
   if (WarnOnAllAutoCopies) {
@@ -73,6 +85,8 @@
       utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (!Expensive || !*Expensive)
     return false;
+  if (!isCopyConstructedFromLValueArg(LoopVar, Context))
+    return false;
   auto Diagnostic =
       diag(LoopVar.getLocation(),
            "the loop variable's type is not a reference type; this creates a "
@@ -93,6 +107,8 @@
       utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
     return false;
+  if (!isCopyConstructedFromLValueArg(LoopVar, Context))
+    return false;
   // We omit the case where the loop variable is not used in the loop body. E.g.
   //
   // for (auto _ : benchmark_state) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78223.257769.patch
Type: text/x-patch
Size: 2827 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200415/f5bfd033/attachment-0001.bin>