Add forEachArgumentWithParam AST matcher

[flx]

Summary

The new matcher allows users to provide a matcher for both the argument of a CallExpr/CxxConstructExpr a well as the ParmVarDecl of the argument.

[klimek]

include/clang/ASTMatchers/ASTMatchers.h

2825–2851 ↗

(On Diff #37680)

I don't think the handling of BoundNodesTreeBuilder is correct (writing tests to catch that will be a nice exercise ;)

The trick is that the resulting BoundNodesTreeBuilder must contain the full results of all matches.

If you look at the other forEach matchers, you can see a pattern:
a) create a new empty BoundNodesTreeBuilder 'Result'
b) for each match you try, create a new BoundNodesTreeBuilder with a copy of the current one
c) call match with that builder
d) for the result that matches, call addMatch

If you don't pass the builder in that is initialized with the current result, any subexpressions that uses equalsBoundNode will be incorrect.

Please add tests that test equalsBoundNode behavior in all cases, especially if you have multiple ones where everything matches, but then the equalsBoundNode fails.

[flx]

I think I'm not yet fully clean on the semantics of matching bound nodes. Do we want to allow that also between the argument matcher and the param matcher by copying the matches between the two matchers or do we only support matches to bound nodes that are already matched before forEachArgumetWithParam is invoked?

I'm also not quite sure how write these tests. Looking at some of the other forEach matchers (SwitchCase, ContructorInitializer) I don't see examples I could pattern mine after.

I updated the change to copy the Builder.

[klimek]

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

[sbenza]

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

include/clang/ASTMatchers/ASTMatchers.h

2837 ↗

(On Diff #37872)

use isa<> instead of dyn_cast<> if you don't care about the pointer itself.

Also, you could fold that check into the matcher itself using expr(anyOf(cxxConstructExpr(...), callExpr(...)))

[alexfh]

What's the state here?

[flx]

In D13845#271102, @klimek wrote:

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

Thanks. I've added that test case and am counting 4 matches, two where 'a' is used as argument and two for 'b'.

Let me know if that is sufficient whether you can think of more test cases.

In D13845#271102, @klimek wrote:

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

Thanks for the example. I created a test case based on it. Let me know if any other cases are still missing.

include/clang/ASTMatchers/ASTMatchers.h

2837 ↗

(On Diff #37872)

Thanks, done!

[alexfh]

In D13845#271378, @sbenza wrote:

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

[alexfh]

Sam, Manuel, any other concerns?

[flx]

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the first 0-th argument is actually the implicit object argument and the corresponding matcher should not be applied against parameter 0 which belongs to argument 0.

To handle this I'm incrementing the argument index by one in this case.

[flx]

Sorry, I uploaded the wrong patch. This is fixed now.

[flx]

In D13845#315523, @alexfh wrote:

In D13845#271378, @sbenza wrote:

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

Updated patch with this.

In D13845#315523, @alexfh wrote:

In D13845#271378, @sbenza wrote:

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

In D13845#318127, @flx wrote:

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the first 0-th argument is actually the implicit object argument and the corresponding matcher should not be applied against parameter 0 which belongs to argument 0.

To handle this I'm incrementing the argument index by one in this case.

Sorry, the above comment was confusing (a lot of off-by one errors). Corrected version:

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the 0-th argument is actually the implicit object argument and Argument matcher should not be applied it. Instead argument 1 and parameter 0 should be paired.

To handle this I'm incrementing the argument index by one in this case.

[flx]

Manuel, could you take another look?

[klimek]

LG.

unittests/ASTMatchers/ASTMatchersTest.cpp

1689–1692 ↗

(On Diff #43797)

Nice!

[flx]

Manuel, could you submit the change? I'm still working on becoming a submitter.

[klimek]

include/clang/ASTMatchers/ASTMatchers.h

2899 ↗

(On Diff #43797)

Changing to unsigned due to -Wsign-compare trigger.

clang-tidy/performance/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyPerformanceModule
+  ForRangeCopyCheck.cpp
   PerformanceTidyModule.cpp
   UnnecessaryCopyInitialization.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/performance/ForRangeCopyCheck.h

+//===--- ForRangeCopyCheck.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_FORRANGECOPYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_FORRANGECOPYCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+// A check that detects copied loop variables and suggests using const
+// references.
+class ForRangeCopyCheck : public ClangTidyCheck {
+public:
+  ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  // Checks if the loop variable is a const value and expensive to copy. If so
+  // suggests it be converted to a const reference.
+  bool handleConstValueCopy(const VarDecl &LoopVar, ASTContext &Context);
+  // Checks if the loop variable is a non-const value and whether only
+  // const methods are invoked on it or whether it is only used as a const
+  // reference argument. If so it suggests it be made a const reference.
+  bool handleCopyIsOnlyConstReferenced(const VarDecl &LoopVar,
+                                       const CXXForRangeStmt &ForRange,
+                                       ASTContext &Context);
+
+  const bool WarnOnAllAutoCopies;
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_FORRANGECOPYCHECK_H

clang-tidy/performance/ForRangeCopyCheck.cpp

+//===--- ForRangeCopyCheck.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 "ForRangeCopyCheck.h"
+#include "../utils/LexerUtils.h"
+#include "../utils/TypeTraits.h"
+#include "clang/Lex/Lexer.h"
+
+#include <set>
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+using namespace ::clang::ast_matchers;
+
+namespace {
+
+template <typename S> bool isSetDifferenceEmpty(const S &S1, const S &S2) {
+  for (const auto &E : S1)
+    if (S2.find(E) == S2.end())
+      return false;
+  return true;
+}
+
+// Extracts all Nodes keyed by ID from Matches and inserts them into Nodes.
+template <typename Node>
+void extractNodesByIdTo(const SmallVector<BoundNodes, 1> &Matches, StringRef ID,
+                        std::set<const Node *> &Nodes) {
+  for (const auto &Match : Matches)
+    Nodes.insert(Match.getNodeAs<Node>(ID));
+}
+
+// Finds all DeclRefExprs to VarDecl in Stmt.
+std::set<const DeclRefExpr *>
+declRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) {
+  auto Matches = match(
+      findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")),
+      Stmt, Context);
+  std::set<const DeclRefExpr *> DeclRefs;
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  return DeclRefs;
+}
+
+// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
+// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
+std::set<const DeclRefExpr *> constReferenceDeclRefExprs(const VarDecl &VarDecl,
+                                                         const Stmt &Stmt,
+                                                         ASTContext &Context) {
+  auto DeclRefToVar =
+      declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
+  auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
+  // Match method call expressions where the variable is referenced as the this
+  // implicit object argument and opertor call expression for member operators
+  // where the variable is the 0-th argument.
+  auto Matches = match(
+      findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
+                         cxxOperatorCallExpr(ConstMethodCallee,
+                                             hasArgument(0, DeclRefToVar))))),
+      Stmt, Context);
+  std::set<const DeclRefExpr *> DeclRefs;
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  auto ConstReferenceOrValue =
+      qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))),
+                     unless(anyOf(referenceType(), pointerType()))));
+  auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
+      DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  Matches =
+      match(findAll(cxxConstructExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
+  extractNodesByIdTo(Matches, "declRef", DeclRefs);
+  return DeclRefs;
+}
+
+// Modifies VarDecl to be a reference.
+FixItHint createAmpersandFix(const VarDecl &VarDecl, ASTContext &Context) {
+  SourceLocation AmpLocation = VarDecl.getLocation();
+  auto Token = lexer_utils::getPreviousNonCommentToken(Context, AmpLocation);
+  if (!Token.is(tok::unknown))
+    AmpLocation = Token.getLocation().getLocWithOffset(Token.getLength());
+  return FixItHint::CreateInsertion(AmpLocation, "&");
+}
+
+// Modifies VarDecl to be const.
+FixItHint createConstFix(const VarDecl &VarDecl) {
+  return FixItHint::CreateInsertion(VarDecl.getTypeSpecStartLoc(), "const ");
+}
+
+} // namespace
+
+ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)) {}
+
+void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies);
+}
+
+void ForRangeCopyCheck::registerMatchers(MatchFinder *Finder) {
+  auto LoopVar =
+      varDecl(hasType(qualType(unless(anyOf(referenceType(), pointerType())))));
+  Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar")))
+                         .bind("forRange"),
+                     this);
+}
+
+void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Var = Result.Nodes.getNodeAs<VarDecl>("loopVar");
+  const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange");
+  if (handleConstValueCopy(*Var, *Result.Context))
+    return;
+  handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context);
+}
+
+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;
+  }
+  if (!type_traits::isExpensiveToCopy(LoopVar.getType(), 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")
+      << createAmpersandFix(LoopVar, Context);
+  if (!LoopVar.getType().isConstQualified())
+    Diagnostic << createConstFix(LoopVar);
+  return true;
+}
+
+bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
+    const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
+    ASTContext &Context) {
+  if (LoopVar.getType().isConstQualified() ||
+      !type_traits::isExpensiveToCopy(LoopVar.getType(), Context)) {
+    return false;
+  }
+  // Collect all DeclRefExprs to the loop variable and all CallExprs and
+  // CXXConstructExprs where the loop variable is used as argument to a const
+  // reference parameter.
+  // If the difference is empty it is safe for the loop variable to be a const
+  // reference.
+  auto AllDeclRefs = declRefExprs(LoopVar, *ForRange.getBody(), Context);
+  auto ConstReferenceDeclRefs =
+      constReferenceDeclRefExprs(LoopVar, *ForRange.getBody(), Context);
+  if (AllDeclRefs.empty() ||
+      !isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs))
+    return false;
+  diag(LoopVar.getLocation(),
+       "loop variable is copied but only used as const reference; consider "
+       "making it a const reference")
+      << createConstFix(LoopVar) << createAmpersandFix(LoopVar, Context);
+  return true;
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang

clang-tidy/performance/PerformanceTidyModule.cpp

 //===--- PeformanceTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 
+#include "ForRangeCopyCheck.h"
 #include "UnnecessaryCopyInitialization.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 class PerformanceModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ForRangeCopyCheck>("performance-for-range-copy");
     CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
         "performance-unnecessary-copy-initialization");
   }
 };
 
 // Register the PerformanceModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<PerformanceModule>
     X("performance-module", "Adds performance checks.");

test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp

+// RUN: %check_clang_tidy %s performance-for-range-copy %t -config="{CheckOptions: [{key: "performance-for-range-copy.WarnOnAllAutoCopies", value: 1}]}" -- -std=c++11
+
+template <typename T>
+struct Iterator {
+  void operator++() {}
+  T operator*() { return T(); }
+  bool operator!=(const Iterator &) { return false; }
+};
+template <typename T>
+struct View {
+  T begin() { return T(); }
+  T begin() const { return T(); }
+  T end() { return T(); }
+  T end() const { return T(); }
+};
+
+struct S {
+  S();
+  S(const S &);
+  ~S();
+  S &operator=(const S &);
+};
+
+void NegativeLoopVariableNotAuto() {
+  for (S S1 : View<Iterator<S>>()) {
+    S* S2 = &S1;
+  }
+}
+
+void PositiveTriggeredForAutoLoopVariable() {
+  for (auto S1 : View<Iterator<S>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: 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 auto& S1 : View<Iterator<S>>()) {
+    S* S2 = &S1;
+  }
+}

test/clang-tidy/performance-for-range-copy.cpp

+// RUN: %check_clang_tidy %s performance-for-range-copy %t
+
+namespace std {
+
+template <typename _Tp>
+struct remove_reference { typedef _Tp type; };
+
+template <typename _Tp>
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
+}
+
+} // std
+
+template <typename T>
+struct Iterator {
+  void operator++() {}
+  T operator*() { return T(); }
+  bool operator!=(const Iterator &) { return false; }
+};
+template <typename T>
+struct View {
+  T begin() { return T(); }
+  T begin() const { return T(); }
+  T end() { return T(); }
+  T end() const { return T(); }
+};
+
+struct S {
+  S();
+  S(const S &);
+  ~S();
+  S &operator=(const S &);
+};
+
+void NegativeConstReference() {
+  for (const S &S1 : View<Iterator<S>>()) {
+  }
+}
+
+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>>()) {}
+
+  // Don't warn on dependent types.
+  for (const T t1 : View<Iterator<T>>()) {
+  }
+}
+
+template <typename T>
+void instantiated() {
+  for (const S S2 : View<Iterator<S>>()) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}}
+  // CHECK-FIXES: {{^}}  for (const S& S2 : View<Iterator<S>>()) {}
+
+  for (const T T2 : View<Iterator<T>>()) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}}
+  // CHECK-FIXES: {{^}}  for (const T& T2 : View<Iterator<T>>()) {}
+}
+
+void f() {
+  instantiated<int>();
+  instantiated<S>();
+}
+
+struct Mutable {
+  Mutable() {}
+  Mutable(const Mutable &) = default;
+  Mutable(const Mutable &, const Mutable &) {}
+  void setBool(bool B) {}
+  bool constMethod() const {
+    return true;
+  }
+  Mutable& operator[](int I) {
+    return *this;
+  }
+  bool operator==(const Mutable &Other) const {
+    return true;
+  }
+  ~Mutable() {}
+};
+
+Mutable& operator<<(Mutable &Out, bool B) {
+  Out.setBool(B);
+  return Out;
+}
+
+bool operator!=(const Mutable& M1, const Mutable& M2) {
+  return false;
+}
+
+void use(const Mutable &M);
+void useTwice(const Mutable &M1, const Mutable &M2);
+void useByValue(Mutable M);
+void useByConstValue(const Mutable M);
+void mutate(Mutable *M);
+void mutate(Mutable &M);
+void onceConstOnceMutated(const Mutable &M1, Mutable &M2);
+
+void negativeVariableIsMutated() {
+  for (auto M : View<Iterator<Mutable>>()) {
+    mutate(M);
+  }
+  for (auto M : View<Iterator<Mutable>>()) {
+    mutate(&M);
+  }
+  for (auto M : View<Iterator<Mutable>>()) {
+    M.setBool(true);
+  }
+}
+
+void negativeOnceConstOnceMutated() {
+  for (auto M : View<Iterator<Mutable>>()) {
+    onceConstOnceMutated(M, M);
+  }
+}
+
+void negativeVarIsMoved() {
+  for (auto M : View<Iterator<Mutable>>()) {
+    auto Moved = std::move(M);
+  }
+}
+
+void negativeNonConstOperatorIsInvoked() {
+  for (auto NonConstOperatorInvokee : View<Iterator<Mutable>>()) {
+    auto& N = NonConstOperatorInvokee[0];
+  }
+}
+
+void negativeNonConstNonMemberOperatorInvoked() {
+  for (auto NonConstOperatorInvokee : View<Iterator<Mutable>>()) {
+    NonConstOperatorInvokee << true;
+  }
+}
+
+void positiveOnlyConstMethodInvoked() {
+  for (auto M : View<Iterator<Mutable>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
+    // CHECK-FIXES: for (const auto& M : View<Iterator<Mutable>>()) {
+    M.constMethod();
+  }
+}
+
+void positiveOnlyUsedAsConstArguments() {
+  for (auto UsedAsConst : View<Iterator<Mutable>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
+    // CHECK-FIXES: for (const auto& UsedAsConst : View<Iterator<Mutable>>()) {
+    use(UsedAsConst);
+    useTwice(UsedAsConst, UsedAsConst);
+    useByValue(UsedAsConst);
+    useByConstValue(UsedAsConst);
+  }
+}
+
+void positiveOnlyUsedInCopyConstructor() {
+  for (auto A : View<Iterator<Mutable>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
+    // CHECK-FIXES: for (const auto& A : View<Iterator<Mutable>>()) {
+    Mutable Copy = A;
+    Mutable Copy2(A);
+  }
+}
+
+void positiveTwoConstConstructorArgs() {
+  for (auto A : View<Iterator<Mutable>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
+    // CHECK-FIXES: for (const auto& A : View<Iterator<Mutable>>()) {
+    Mutable Copy(A, A);
+  }
+}
+
+void PositiveConstMemberOperatorInvoked() {
+  for (auto ConstOperatorInvokee : View<Iterator<Mutable>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
+    // CHECK-FIXES: for (const auto& ConstOperatorInvokee : View<Iterator<Mutable>>()) {
+    bool result = ConstOperatorInvokee == Mutable();
+  }
+}
+
+void PositiveConstNonMemberOperatorInvoked() {
+  for (auto ConstOperatorInvokee : View<Iterator<Mutable>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
+    // CHECK-FIXES: for (const auto& ConstOperatorInvokee : View<Iterator<Mutable>>()) {
+    bool result = ConstOperatorInvokee != Mutable();
+  }
+}