Add bugprone-loop-variable-copied-then-modified clang-tidy check.

[flowerhack]

Adds a clang-tidy check that alerts when a loop variable is copied and subsequently modified.

This is a bugprone pattern because the programmer in this case often assumes they are modifying the original value instead of a copy.

This warning can be suppressed by either converting the loop variable to a const ref, or by performing the copy explicitly inside the body of the loop.

Fix GH-155922

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[flowerhack]

Adressing GH-155922.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Julia Hansbrough (flowerhack)

Changes

Adds a clang-tidy check that alerts when a loop variable is copied and subsequently modified.

This is a bugprone pattern because the programmer in this case often assumes they are modifying the original value instead of a copy.

This warning can be suppressed by either converting the loop variable to a const ref, or by performing the copy explicitly inside the body of the loop.

---

Full diff: https://github.com/llvm/llvm-project/pull/157213.diff

8 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3)
• (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1)
• (added) clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.cpp (+80)
• (added) clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.h (+37)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+8)
• (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.rst (+41)
• (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+2-1)
• (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified.cpp (+50)

diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 824ebdfbd00dc..3ce32d88ea005 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -40,6 +40,7 @@
 #include "IntegerDivisionCheck.h"
 #include "InvalidEnumDefaultInitializationCheck.h"
 #include "LambdaFunctionNameCheck.h"
+#include "LoopVariableCopiedThenModifiedCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisleadingSetterOfReferenceCheck.h"
@@ -153,6 +154,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-incorrect-enable-if");
     CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>(
         "bugprone-incorrect-enable-shared-from-this");
+    CheckFactories.registerCheck<LoopVariableCopiedThenModifiedCheck>(
+        "bugprone-loop-variable-copied-then-modified");
     CheckFactories.registerCheck<UnintendedCharOstreamOutputCheck>(
         "bugprone-unintended-char-ostream-output");
     CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 59928e5e47a09..fb28f075b991e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -31,6 +31,7 @@ add_clang_library(clangTidyBugproneModule STATIC
   IncorrectEnableIfCheck.cpp
   IncorrectEnableSharedFromThisCheck.cpp
   InvalidEnumDefaultInitializationCheck.cpp
+  LoopVariableCopiedThenModifiedCheck.cpp
   UnintendedCharOstreamOutputCheck.cpp
   ReturnConstRefFromParameterCheck.cpp
   SuspiciousStringviewDataUsageCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.cpp
new file mode 100644
index 0000000000000..38fe7c5a5215d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.cpp
@@ -0,0 +1,80 @@
+
+//===----------------------------------------------------------------------===//
+//
+// 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 "LoopVariableCopiedThenModifiedCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
+#include "clang/Basic/Diagnostic.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void LoopVariableCopiedThenModifiedCheck::registerMatchers(MatchFinder *Finder) {
+  auto HasReferenceOrPointerTypeOrIsAllowed = hasType(qualType(
+      unless(hasCanonicalType(anyOf(referenceType(), pointerType())))));
+  auto IteratorReturnsValueType = cxxOperatorCallExpr(
+      hasOverloadedOperatorName("*"),
+      callee(
+          cxxMethodDecl(returns(unless(hasCanonicalType(referenceType()))))));
+  auto NotConstructedByCopy = cxxConstructExpr(
+      hasDeclaration(cxxConstructorDecl(unless(isCopyConstructor()))));
+  auto ConstructedByConversion = cxxMemberCallExpr(callee(cxxConversionDecl()));
+  auto LoopVar =
+      varDecl(HasReferenceOrPointerTypeOrIsAllowed,
+              unless(hasInitializer(expr(hasDescendant(expr(
+                  anyOf(materializeTemporaryExpr(), IteratorReturnsValueType,
+                        NotConstructedByCopy, ConstructedByConversion)))))));
+  Finder->addMatcher(
+      traverse(TK_AsIs,
+               cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar")))
+                   .bind("forRange")),
+      this);
+}
+
+void LoopVariableCopiedThenModifiedCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Var = Result.Nodes.getNodeAs<VarDecl>("loopVar");
+  if (Var->getBeginLoc().isMacroID())
+    return;
+  const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange");
+  if (copiedLoopVarIsMutated(*Var, *ForRange, *Result.Context))
+    return;
+}
+
+bool LoopVariableCopiedThenModifiedCheck::copiedLoopVarIsMutated(const VarDecl &LoopVar,
+                                               const CXXForRangeStmt &ForRange,
+                                               ASTContext &Context) {
+
+  std::string hintstring = "";
+
+  if (ExprMutationAnalyzer(*ForRange.getBody(), Context)
+          .isMutated(&LoopVar)) {
+    if (isa<AutoType>(LoopVar.getType())) {
+      hintstring = "const auto&";
+    }
+    else {
+      std::string CanonicalTypeStr = LoopVar.getType().getAsString(Context.getLangOpts());
+      hintstring = "const " + CanonicalTypeStr + "&";
+    }
+    clang::SourceRange loopvar_source_range = LoopVar.getTypeSourceInfo()->getTypeLoc().getSourceRange();
+    auto Diag =
+        diag(LoopVar.getLocation(), "loop variable '%0' is copied and then "
+                                    "modified, which is likely a bug; you "
+                                    "probably want to modify the underlying "
+                                    "object and not this copy. If you "
+                                    "*did* intend to modify this copy, "
+                                    "please use an explicit copy inside the "
+                                    "body of the loop") << LoopVar.getName() << FixItHint::CreateReplacement(loopvar_source_range, hintstring);
+    return true;
+  }
+  return false;
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.h b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.h
new file mode 100644
index 0000000000000..3f9f55ffdaf83
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/LoopVariableCopiedThenModifiedCheck.h
@@ -0,0 +1,37 @@
+
+//===----------------------------------------------------------------------===//
+//
+// 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_BUGPRONE_LOOPVARIABLECOPIEDTHENMODIFIEDCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LOOPVARIABLECOPIEDTHENMODIFIEDCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds loop variables that are copied and subsequently modified.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.html
+class LoopVariableCopiedThenModifiedCheck : public ClangTidyCheck {
+public:
+  LoopVariableCopiedThenModifiedCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  bool copiedLoopVarIsMutated(const VarDecl &LoopVar,
+                              const CXXForRangeStmt &ForRange,
+                              ASTContext &Context);
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_LOOPVARIABLECOPIEDTHENMODIFIEDCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 0f230b8fbdebd..bc95f18c04e84 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -139,6 +139,14 @@ New checks
   Detects default initialization (to 0) of variables with ``enum`` type where
   the enum has no enumerator with value of 0.
 
+- New :doc:`bugprone-loop-variable-copied-then-modified
+  <clang-tidy/checks/bugprone/loop-variable-copied-then-modified>` check.
+
+  Detects when a loop variable is copied and then subsequently modified.
+  Suggests replacing such instances with either a const ref (to prevent the
+  copy) or by performing the copy explicitly inside the loop (to make it
+  obvious one intends to modify a copy instead of the underlying object).
+
 - New :doc:`cppcoreguidelines-pro-bounds-avoid-unchecked-container-access
   <clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-access>`
   check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.rst
new file mode 100644
index 0000000000000..fa04ae33989cc
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/loop-variable-copied-then-modified.rst
@@ -0,0 +1,41 @@
+.. title:: clang-tidy - bugprone-loop-variable-copied-then-modified
+
+bugprone-loop-variable-copied-then-modified
+===========================================
+
+Warns when a loop variable is copied and subsequently modified.
+
+This pattern is considered bugprone because, frequently, programmers do not
+realize that they are modifying a *copy* rather than an underlying value,
+resulting in subtly erroneous code.
+
+For instance, the following code attempts to null out a value in a map, but only
+succeeds in 
+
+.. code-block:: c++
+
+  for (auto target : target_map) {
+    target.value = nullptr;
+  }
+
+The programmer is likely to have intended this code instead:
+
+.. code-block:: c++
+    
+  for (const auto& target : target_map) {
+    target.value = nullptr;
+  }
+
+This warning can be suppressed in one of two ways:
+  - In cases where the programmer did not intend to create a copy, they can
+    convert the loop variable to a const reference. A FixIt message will
+    provide a naive suggestion of how to achieve this, which works in most
+    cases.
+  - In cases where the intent is in fact to modify a copy, they may perform the
+    copy inside the body of the loop, and perform whatever operations they like
+    on that copy.
+
+This is a conservative check: in cases where it cannot be determined at compile
+time whether or not a particular function modifies the variable, it assumes a
+modification has ocurred and warns accordingly. However, in such cases, the
+warning will still be suppressed by doing one of the actions described above.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 5e3ffc4f8aca3..bb88a4d3e05b1 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -108,6 +108,7 @@ Clang-Tidy Checks
    :doc:`bugprone-integer-division <bugprone/integer-division>`,
    :doc:`bugprone-invalid-enum-default-initialization <bugprone/invalid-enum-default-initialization>`,
    :doc:`bugprone-lambda-function-name <bugprone/lambda-function-name>`,
+   :doc:`bugprone-loop-variable-copied-then-modified <bugprone/loop-variable-copied-then-modified>`, "Yes"
    :doc:`bugprone-macro-parentheses <bugprone/macro-parentheses>`, "Yes"
    :doc:`bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects>`,
    :doc:`bugprone-misleading-setter-of-reference <bugprone/misleading-setter-of-reference>`,
@@ -249,12 +250,12 @@ Clang-Tidy Checks
    :doc:`linuxkernel-must-check-errs <linuxkernel/must-check-errs>`,
    :doc:`llvm-header-guard <llvm/header-guard>`,
    :doc:`llvm-include-order <llvm/include-order>`, "Yes"
-   :doc:`llvm-use-new-mlir-op-builder <llvm/use-new-mlir-op-builder>`, "Yes"
    :doc:`llvm-namespace-comment <llvm/namespace-comment>`,
    :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm/prefer-isa-or-dyn-cast-in-conditionals>`, "Yes"
    :doc:`llvm-prefer-register-over-unsigned <llvm/prefer-register-over-unsigned>`, "Yes"
    :doc:`llvm-prefer-static-over-anonymous-namespace <llvm/prefer-static-over-anonymous-namespace>`,
    :doc:`llvm-twine-local <llvm/twine-local>`, "Yes"
+   :doc:`llvm-use-new-mlir-op-builder <llvm/use-new-mlir-op-builder>`, "Yes"
    :doc:`llvmlibc-callee-namespace <llvmlibc/callee-namespace>`,
    :doc:`llvmlibc-implementation-in-namespace <llvmlibc/implementation-in-namespace>`,
    :doc:`llvmlibc-inline-function-decl <llvmlibc/inline-function-decl>`, "Yes"
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified.cpp
new file mode 100644
index 0000000000000..924738f14a86a
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/loop-variable-copied-then-modified.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s bugprone-loop-variable-copied-then-modified %t
+
+template <typename T>
+struct Iterator {
+  void operator++() {}
+  const T& operator*() {
+    static T* TT = new T();
+    return *TT;
+  }
+  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 {
+  int value;
+
+  S() : value(0) {};
+  S(const S &);
+  ~S();
+  S &operator=(const S &);
+  void modify() {
+    value++;
+  }
+};
+
+void NegativeLoopVariableNotCopied() {
+  for (const S& S1 : View<Iterator<S>>()) {
+    // It's fine to copy-by-value S1 into some other S.
+    S S2 = S1;
+  }
+}
+
+void NegativeLoopVariableCopiedButNotModified() {
+  for (S S1 : View<Iterator<S>>()) {
+  }
+}
+
+void PositiveLoopVariableCopiedAndThenModfied() {
+  for (S S1 : View<Iterator<S>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:10: warning: loop variable 'S1' is copied and then modified, which is likely a bug; you probably want to modify the underlying object and not this copy. If you *did* intend to modify this copy, please use an explicit copy inside the body of the loop
+    // CHECK-FIXES: for (const S& S1 : View<Iterator<S>>()) {
+    S1.modify();
+  }
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[zwuis]

Adressing GH-155922.

You can add Fix GH-155922or something like this to PR description so that the issue will be closed when the PR is merged. See https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue.

[flowerhack]

Thanks for the feedback; I believe I've addressed all comments, save one comment from @zwuis which I requested clarification on. Let me know if there's anything else.

[nicovank]

Thanks!

I'm not a fan of the FixIt often leading to invalid code. Suggesting const when the variable is modified in the loop always produces invalid code (this is the case for the highlighted example in the documentation): suggesting const makes little sense when we know/assume the variable is modified.

Even if switching to reference only, the check will always transform the behavior of user code if applied directly. Most checks perform non-behavior-changing transformations. Maybe the FixIt can be a note instead or removed?

I ran this check on CMake sources (medium-sized codebase), there are a few true positives but also many false positives. Many being strings that are manipulated before being re-inserted into a different container for example. I couldn't think of a solution to reduce those yet.

One idea maybe is to check if the copy is then used after modification. If it is not, that's a strong signal the copy was unintended. But this seems tricky.

[nicovank]

Is this actually better and/or more readable than having the copy as the loop variable? I'm leaning towards no. Which is also a problem with suggesting that in the diagnostic.

[flowerhack]

I'd argue that it is better and more readable, in most cases: based on code we've observed in Chromium, it's a surprisingly-common footgun for developers to perform a copy in the loop declaration without realizing it. Moving that copy inside the body of the loop takes behavior that is implicit and makes it explicit, difficult-to-ignore, demands-the-programmer-has-thought-through-what-they're-doing, etc. See e.g. https://chromium-review.googlesource.com/c/chromium/src/+/6409957 and https://chromium-review.googlesource.com/c/chromium/src/+/6512516 —in both cases it seems unlikely the bug would've gone unnoticed if they'd been forced to either make it a reference or make the copy explicit.

[flowerhack]

Let me know if it'd be OK to resolve this conversation; thanks!

[flowerhack]

@nicovank : Thanks for the thoughtful and detailed comments & review. Replying to your toplevel comment here:

Suggesting const when the variable is modified in the loop always produces invalid code (this is the case for the highlighted example in the documentation): suggesting const makes little sense when we know/assume the variable is modified.

I performed an experiment where I ran this check over the Chromium codebase. After filtering out uninteresting code (*_test.cc files, etc), it returned about 200 results. Of those results, I picked about 50 to try handling by applying the naive fix—"if I switch this to a const ref, does it still compile?"—and in 70% of cases, the naive fix compiled without incident. (I believe that the disparity between "things that the ExprMutationAnalyzer think represents a mutation" vs "things that the const designation thinks represents a mutation" is due to the ExprMutationAnalyzer's more conservative semantics.)

The remaining cases, to my eye, seemed like complicated enough situations that they would in fact benefit from more thought/care from the programmer, and it seems fine that the fix-it won't work out-of-the-box for those situations (generally require a judgment call from the programmer to either switch the variable in the loop to a non-const reference, or make the copy explicit in the loop).

So, my hope with the fix-it would be to offer a simple suggestion that would work "most" of the time to minimize disruption to developers.

That does mean some fixes suggested by this will not necessarily be bugfixes but rather a style enforcement—"prefer const references or references over copies in loop variables"— and that does make this something of an opinionated check, to be sure. And the IgnoreInexpensiveVariables toggle does exist for those who'd like some benefit from this check with a higher signal-to-noise ratio. But I do think it's valuable to have a version of the check that's concerned about all variables, since we've observed errors of this type involving inexpensive variables.

One idea maybe is to check if the copy is then used after modification. If it is not, that's a strong signal the copy was unintended. But this seems tricky.

I think such a modification would fail to catch many of the cases we care about, unfortunately. A pretty common pattern is to iterate over some set of objects (performing a copy on each iteration), modify a field in each object, and then continue. In this case the field isn't "used" again inside the body of that function.

One idea maybe is to check if the copy is then used after modification. If it is not, that's a strong signal the copy was unintended. But this seems tricky.

I think such a modification would fail to catch many of the cases we care about, unfortunately. A pretty common pattern is to iterate over some set of objects (performing a copy on each iteration), modify a field in each object, and then continue. In this case the field isn't "used" again inside the body of that function.

[flowerhack]

Also, any chance I could get approval on the "build & test linux" + "build & test windows" workflows (or is there something I need to do to get those approved)? I've tested on my machine but would like to know things check out against CI as well. Thanks!

[vbvictor]

Suggested change

	// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-loop-variable-copied-then-modified %t
	// RUN: %check_clang_tidy %s bugprone-loop-variable-copied-then-modified %t

c++11 is the default - I don't see why it needs c++17

[flowerhack]

C++17 has class template argument deduction, and I thought it'd be a useful test case to see if the FixIt does the correct thing in that case. I can change that or put that case in a separate file, though, if you'd prefer? Just let me know.

[vbvictor]

You should make 2 RUN commands for different C++ standarts.
Since you target generic C++, then first one should be
--std=c++98,c++03,c++11,c++14, another one --std=c++17-or-later.
Look for other test files that have multiple RUN commands.

I think for --std=c++17-or-later RUN command specifying -check-suffix=,CPP17 would do the trick, like in clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-goto.cpp

[flowerhack]

I think I'm still confused: when looking at other files with multiple RUN declarations (I'm looking at bugprone/dangling-handle.cpp and unsafe-functions.c), it seems like it's designed for situations where you'd like to compile the same code but with different options.

The problem is that e.g. the PositiveLoopVariableCopiedAndThenModfiedGeneric function won't compile at all in earlier versions of C++; I get:

"error: use of class template 'Generic' requires template arguments [clang-diagnostic-error]"

So I assume I should move the C++17-specific code to a separate file; I've done that in this most recent update, and left the original file as generic/unspecified.

I also don't think the check-suffix suggestion works—or at least, i wasn't able to get it to work. When I used this run function:

// RUN: %check_clang_tidy -check-suffix=,CPP17 %s bugprone-loop-variable-copied-then-modified %t --fix-notes

I got the error:

# | CHECK-FIXES-CPP17, CHECK-MESSAGES-CPP17 or CHECK-NOTES-CPP17 not found in the input

So I've left it as --std=c++17-or-later, but please let me know if I'm holding it wrong and if so how to fix it!

[flowerhack]

(Stepping out for a couple hours, but I'll be adding the tests very shortly after that & will add another comment here when this is ready for another round of review. Thanks!)

[flowerhack]

I'm aware this message is a little odd. It does point to the right part of the code and suggest the right fix. I found myself wavering over the best way to make this more precise—it would require moving a lot of code that's currently in the matcher into the check, since as-of-right-now I don't think it's possible to determine whether LoopVar belongs to a binding declaration within the body of check. I think the intent is clear enough as-is. If there is a desire for a more precise message, however, I'd prefer to do that refactoring in a follow-on CL / as a TODO if possible. (If not, I'd appreciate a second opinion on whether it's okay to move more code from the matcher into check.)

[flowerhack]

This should be ready for another round of review!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code linter.

[flowerhack]

Hi! Just wondering if there's anything else you need from me here?