[clang-tidy] performance-range-for-copy only for copy.

[EricWF]

Summary

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

[aaron.ballman]

LGTM aside from some small nits

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

68

Formatting is off here, so run through clang-format?

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

50

Spurious whitespace change?

[xgupta]

Reverse ping, this patch was never gets committed after being accepted.

[PiotrZSL]

Release notes are missing, and patch not not apply gracefully.

[PiotrZSL]

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

66–67

Those tests already exist and pass in current version.
Most probably this change is obsolete.

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

void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) {
   if (Var->getBeginLoc().isMacroID())
     return;
   if (handleConstValueCopy(*Var, *Result.Context))
     return;
   const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange");
   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) {
     // For aggressive check just test that loop variable has auto type.
     if (!isa<AutoType>(LoopVar.getType()))
       return false;
   } else if (!LoopVar.getType().isConstQualified()) {
     return false;
   }
   llvm::Optional<bool> Expensive =
       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 "
            "copy in each iteration; consider making this a reference")
       << utils::fixit::changeVarDeclToReference(LoopVar, Context);
   if (!LoopVar.getType().isConstQualified()) {
     if (llvm::Optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
             LoopVar, Context, DeclSpec::TQ::TQ_const))
       Diagnostic << *Fix;
   }
   return true;
 }
 
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
     ASTContext &Context) {
   llvm::Optional<bool> Expensive =
       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) {
   // }
   //
   // Because the fix (changing to `const auto &`) will introduce an unused
   // compiler warning which can't be suppressed.
   // Since this case is very rare, it is safe to ignore it.

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

 // 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 {
 
 template <typename _Tp>
 struct remove_reference { typedef _Tp type; };
 template <typename _Tp>
 struct remove_reference<_Tp&> { typedef _Tp type; };
 template <typename _Tp>

 struct S {
   S();
   S(const S &);
   S(const ConstructorConvertible&) {}
   ~S();
   S &operator=(const S &);
 };
 
+
 struct Convertible {
   operator S() const {
     return S();
   }
 };
 
 void negativeConstReference() {
   for (const S &S1 : View<Iterator<S>>()) {
   }
 }
 
 void negativeUserDefinedConversion() {
   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>
 void uninstantiated() {
   for (const S S1 : View<Iterator<S>>()) {}
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
   // CHECK-FIXES: {{^}}  for (const S& S1 : View<Iterator<S>>()) {}