llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Julia Hansbrough (flowerhack) <details> <summary>Changes</summary> …opy. Adds an option 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. The warning can be suppressed by instead explicitly copying the value inside the body of the loop. --- Full diff: https://github.com/llvm/llvm-project/pull/155731.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp (+30-2) - (modified) clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h (+8) - (modified) clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst (+12) - (added) clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp (+49) ``````````diff diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp index f545a49dc184b..cbb842927bd57 100644 --- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp @@ -22,11 +22,15 @@ namespace clang::tidy::performance { ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", false)), + WarnOnModificationOfCopiedLoopVariable( + Options.get("WarnOnModificationOfCopiedLoopVariable", false)), AllowedTypes( utils::options::parseStringList(Options.get("AllowedTypes", ""))) {} void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies); + Options.store(Opts, "WarnOnModificationOfCopiedLoopVariable", + WarnOnModificationOfCopiedLoopVariable); Options.store(Opts, "AllowedTypes", utils::options::serializeStringList(AllowedTypes)); } @@ -64,9 +68,11 @@ void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) { // Ignore code in macros since we can't place the fixes correctly. if (Var->getBeginLoc().isMacroID()) return; + const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange"); + if (copiedLoopVarIsMutated(*Var, *ForRange, *Result.Context)) + return; if (handleConstValueCopy(*Var, *Result.Context)) return; - const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange"); handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context); } @@ -79,6 +85,7 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar, } else if (!LoopVar.getType().isConstQualified()) { return false; } + std::optional<bool> Expensive = utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context); if (!Expensive || !*Expensive) @@ -105,6 +112,27 @@ static bool isReferenced(const VarDecl &LoopVar, const Stmt &Stmt, .empty(); } +bool ForRangeCopyCheck::copiedLoopVarIsMutated(const VarDecl &LoopVar, + const CXXForRangeStmt &ForRange, + ASTContext &Context) { + // If it's copied and mutated, there's a high chance that's a bug. + if (WarnOnModificationOfCopiedLoopVariable) { + 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; +} + bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( const VarDecl &LoopVar, const CXXForRangeStmt &ForRange, ASTContext &Context) { @@ -112,6 +140,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( 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) { @@ -130,7 +159,6 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced( if (std::optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl( LoopVar, Context, Qualifiers::Const)) Diag << *Fix << utils::fixit::changeVarDeclToReference(LoopVar, Context); - return true; } return false; diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h index 8fabbfa2ae7ba..722ca2f3a2e24 100644 --- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h +++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h @@ -39,8 +39,16 @@ class ForRangeCopyCheck : public ClangTidyCheck { const CXXForRangeStmt &ForRange, ASTContext &Context); + // Checks if the loop variable is copied and then subsequently mutated + // in the body of the loop. If so it suggests the copy was unintentional, + // or, that the copy would be more clear if done inside the body of the loop. + bool copiedLoopVarIsMutated(const VarDecl &LoopVar, + const CXXForRangeStmt &ForRange, + ASTContext &Context); + const bool WarnOnAllAutoCopies; const std::vector<StringRef> AllowedTypes; + const bool WarnOnModificationOfCopiedLoopVariable; }; } // namespace clang::tidy::performance diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst index d740984029482..a8dbf6e3d435a 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst @@ -18,6 +18,12 @@ following heuristic is employed: invoked on it, or it is used as const reference or value argument in constructors or function calls. +There is an additional option to warn any time a loop variable is copied in each +iteration and subsequently modified in the body of the loop. This is likely to +be a bug, since the user may believe they are modifying the underlying value +instead of a copy, and a user that truly intends to modify a copy may do so by +performing the copy explicitly inside the body of the loop. + Options ------- @@ -26,6 +32,12 @@ Options When `true`, warns on any use of ``auto`` as the type of the range-based for loop variable. Default is `false`. +.. option:: WarnOnModificationOfCopiedLoopVariable + + When `true`, warns when the loop variable is copied and subsequently + modified. This is likely to be a bug. Moving the copy to an explicit copy + inside of the loop will suppress the warning. Default is `false`. + .. option:: AllowedTypes A semicolon-separated list of names of types allowed to be copied in each diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp new file mode 100644 index 0000000000000..ed34a4fc586c0 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp @@ -0,0 +1,49 @@ +// RUN: %check_clang_tidy %s performance-for-range-copy %t -- \ +// RUN: -config="{CheckOptions: {performance-for-range-copy.WarnOnModificationOfCopiedLoopVariable: true}}" + +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>>()) { + } +} + +void NegativeLoopVariableCopiedButNotModified() { + for (S S1 : View<Iterator<S>>()) { + } +} + +void PositiveLoopVariableCopiedAndThenModfied() { + for (S S1 : View<Iterator<S>>()) { + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: 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 + S1.modify(); + } +} + `````````` </details> https://github.com/llvm/llvm-project/pull/155731 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
