From a4da9f3e8e4eb71e9a61f5cd6b48a7ec0eb94cc0 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Fri, 15 Aug 2025 21:48:25 +0000 Subject: [PATCH] Add a mode to performance-for-range-copy that alerts when a loop variable is copied and modified. This is a bugprone pattern because the programmer in this case often assumes they are modifying the original value instead of a copy. --- .../performance/ForRangeCopyCheck.cpp | 37 ++++++-- playground/test.cpp | 88 +++++++++++++++++++ 2 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 playground/test.cpp diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp index f545a49dc184b..725889f97b783 100644 --- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp @@ -79,10 +79,15 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar, } else if (!LoopVar.getType().isConstQualified()) { return false; } - std::optional Expensive = - utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context); - if (!Expensive || !*Expensive) - return false; + + // TODO: Check if this is actually needed for some cases? It seems redundant + // with the Expensive check in handleCopyIsOnlyConstReferenced but maybe + // I'm missing something + //std::optional Expensive = + // utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context); + //if (!Expensive || !*Expensive) + // return false; + auto Diagnostic = diag(LoopVar.getLocation(), "the loop variable's type is not a reference type; this creates a " @@ -108,10 +113,16 @@ static bool isReferenced(const VarDecl &LoopVar, const Stmt &Stmt, bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( const VarDecl &LoopVar, const CXXForRangeStmt &ForRange, ASTContext &Context) { - std::optional Expensive = - utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context); - if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive) - return false; + + // TODO: Add some toggle here for caring about Expensive. Legacy checks + // care about this but our new check does not. + // Simply disabling the Expensive check altogether for now so we can test the + // new check. + //std::optional Expensive = + // utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context); + //if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive) + // return false; + // We omit the case where the loop variable is not used in the loop body. E.g. // // for (auto _ : benchmark_state) { @@ -133,6 +144,16 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( return true; } + + // If it's copied and mutated, there's a high chance that's a bug. + if (ExprMutationAnalyzer(*ForRange.getBody(), Context).isMutated(&LoopVar)) { + auto Diag = diag(LoopVar.getLocation(), + "loop variable 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"); + return true; + } return false; } diff --git a/playground/test.cpp b/playground/test.cpp new file mode 100644 index 0000000000000..61c26b2a9419c --- /dev/null +++ b/playground/test.cpp @@ -0,0 +1,88 @@ +#include +#include +#include + +void ModifyString(std::string *s) { + s->append("!"); +} + +class DataCache { +public: + // This member function declaration will also be matched. + // Okay my code ONLY matched this one. + // But God that's so much better than nothing. + const std::vector& get_cached_items() const; + +private: + std::vector items_; +}; + +const std::vector& get_global_strings(); + +const std::vector& get_default_values() { + static std::vector defaults = {3.14, 2.71, 1.61}; + return defaults; +} + +int main() { + std::cout << "Initializing some_strings.\n"; + std::vector some_strings; + some_strings.push_back("one"); + some_strings.push_back("two"); + some_strings.push_back("three"); + // Three tools: + // * suspicious-copy-in-range-loop (as written right now) + // * performance-for-range-copy with no options + // * performance-for-range-copy with WarnOnAllAutoCopies enabled + + std::cout << "Modify copies; print them as you modify them.\n"; + // Case 1. + // These strings are being COPIED and then MODIFIED, so: + // SHOULD WARN? YES + // DOES WARN? + // suspicious-copy-in-range-loop: YES + // ~ performance-for-range-copy with no options: NO + // performance-for-range-copy with WarnOnAllAutoCopies enabled: YES + for (auto x : some_strings) { + ModifyString(&x); + std::cout << x << "\n"; + //std::cout << &x << "\n"; + } + + // Case 2. + // These strings are being COPIED but left UNMODIFIED, so: + // SHOULD WARN? NO + // DOES WARN? + // ~ suspicious-copy-in-range-loop: YES + // performance-for-range-copy with no options: NO + // ~ performance-for-range-copy with WarnOnAllAutoCopies enabled: YES + for (auto x : some_strings) { + std::cout << "hi\n"; + std::cout << x << "\n"; + //std::cout << &x << "\n"; + } + + // Case 3. + // These strings are being NEITHER COPIED, NOR MODIFIED, so + // SHOULD WARN? NO + // DOES WARN? + // suspicious-copy-in-range-loop: NO + // performance-for-range-copy with no options: NO + // performance-for-range-copy with WarnOnAllAutoCopies enabled: NO + for (auto& x : some_strings) { + std::cout << x << "\n"; + std::cout << &x << "\n"; + } + + // Case 4. + // These strings are being NOT COPIED but referenced, and MODIFIED, so + // SHOULD WARN? NO + // DOES WARN? + // suspicious-copy-in-range-loop: NO + // performance-for-range-copy with no options: NO + // performance-for-range-copy with WarnOnAllAutoCopies enabled: NO + for (auto& x : some_strings) { + x = "modd'd!"; + std::cout << x << "\n"; + } +}