Author: David Meng Date: 2026-08-20T09:56:53+03:00 New Revision: c65f2462f22f5099dae8e8d727a80048faf32d46
URL: https://github.com/llvm/llvm-project/commit/c65f2462f22f5099dae8e8d727a80048faf32d46 DIFF: https://github.com/llvm/llvm-project/commit/c65f2462f22f5099dae8e8d727a80048faf32d46.diff LOG: [clang-tidy] Fix invalid suggestion in `performance-inefficient-algorithm` (#217458) `hasArgument` matches the argument after `IgnoreParenImpCasts()`, so for `#define VALUE (1)` the check bound the `1` inside the parentheses. That range covers only part of the macro expansion, so `Lexer::getSourceText` returned an empty string and the suggested fix came out as `s.find()`. Read the source text from the argument as written instead, and emit no fix when there is still no text for it. Fixes https://github.com/llvm/llvm-project/issues/217457 Added: Modified: clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp clang-tools-extra/docs/ReleaseNotes.md clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp index 0f896f3816f57..c5e66db6f2d89 100644 --- a/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/InefficientAlgorithmCheck.cpp @@ -35,7 +35,7 @@ void InefficientAlgorithmCheck::registerMatchers(MatchFinder *Finder) { const auto Matcher = callExpr( - callee(functionDecl(Algorithms)), + callee(functionDecl(Algorithms)), argumentCountAtLeast(3), hasArgument( 0, cxxMemberCallExpr( callee(cxxMethodDecl(hasName("begin"))), @@ -48,8 +48,7 @@ void InefficientAlgorithmCheck::registerMatchers(MatchFinder *Finder) { hasArgument( 1, cxxMemberCallExpr(callee(cxxMethodDecl(hasName("end"))), on(declRefExpr(hasDeclaration( - equalsBoundNode("IneffContObj")))))), - hasArgument(2, expr().bind("AlgParam"))) + equalsBoundNode("IneffContObj"))))))) .bind("IneffAlg"); Finder->addMatcher(Matcher, this); @@ -100,7 +99,6 @@ void InefficientAlgorithmCheck::check(const MatchFinder::MatchResult &Result) { if (Unordered && AlgDecl->getName().contains("bound")) return; - const auto *AlgParam = Result.Nodes.getNodeAs<Expr>("AlgParam"); const auto *IneffContExpr = Result.Nodes.getNodeAs<Expr>("IneffContExpr"); FixItHint Hint; @@ -132,13 +130,18 @@ void InefficientAlgorithmCheck::check(const MatchFinder::MatchResult &Result) { CharSourceRange::getTokenRange(IneffContExpr->getSourceRange()), SM, LangOpts); const StringRef ParamText = Lexer::getSourceText( - CharSourceRange::getTokenRange(AlgParam->getSourceRange()), SM, - LangOpts); - const std::string ReplacementText = - (llvm::Twine(ContainerText) + (PtrToContainer ? "->" : ".") + - AlgDecl->getName() + "(" + ParamText + ")") - .str(); - Hint = FixItHint::CreateReplacement(CallRange, ReplacementText); + CharSourceRange::getTokenRange(AlgCall->getArg(2)->getSourceRange()), + SM, LangOpts); + // There is no source text for an expression that covers only part of a + // macro expansion. Building the replacement from an empty string would + // incorrectly drop the value. + if (!ParamText.empty()) { + const std::string ReplacementText = + (llvm::Twine(ContainerText) + (PtrToContainer ? "->" : ".") + + AlgDecl->getName() + "(" + ParamText + ")") + .str(); + Hint = FixItHint::CreateReplacement(CallRange, ReplacementText); + } } diag(AlgCall->getBeginLoc(), diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 022fff815aa81..9cead803ad0e5 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -154,6 +154,13 @@ infrastructure are described first, followed by tool-specific sections. `std::initializer_list` constructor, as the braced form could select a diff erent constructor. +- Improved {doc}`performance-inefficient-algorithm + <clang-tidy/checks/performance/inefficient-algorithm>` check by copying the + searched-for value as written rather than stripping its parentheses, which + could produce an invalid fix such as `s.find()` when the value came from a + macro. No fix is offered when the value covers only part of a macro + expansion. + - Improved {doc}`readability-enum-initial-value <clang-tidy/checks/readability/enum-initial-value>` check by adding the {option}`AllowReferencedInitialValues` to support the diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp index dafff8c946bb0..0757ecb603d79 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-algorithm.cpp @@ -83,6 +83,13 @@ int main() { auto c = count(s.begin(), s.end(), 43); // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: this STL algorithm call should be // CHECK-FIXES: auto c = s.count(43); + auto p = std::find(s.begin(), s.end(), (43)); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: this STL algorithm call should be + // CHECK-FIXES: auto p = s.find((43)); + int i = 1, j = 2; + auto q = std::find(s.begin(), s.end(), (i++, j)); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: this STL algorithm call should be + // CHECK-FIXES: auto q = s.find((i++, j)); #define SECOND(x, y, z) y SECOND(q,std::count(s.begin(), s.end(), 22),w); @@ -164,3 +171,34 @@ void g(std::set<Value, Ordering> container, int value) { // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be // CHECK-FIXES: lower_bound(container.begin(), container.end(), value, Ordering()); } + +#define PAREN_VALUE (43) +#define VALUE_OF(x) ((x) + 1) +#define PLAIN_VALUE 43 +#define VAL_AND_END s.end(), 46 + +// The searched-for value is copied as written, so a whole macro expansion is +// kept intact, and so is a range that just starts or ends inside one. A value +// that covers only part of an expansion has no source text of its own, and the +// call is then diagnosed without a fix. +void macroExpansion(std::set<int> s, int i) { + find(s.begin(), s.end(), PAREN_VALUE); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be + // CHECK-FIXES: s.find(PAREN_VALUE); + + count(s.begin(), s.end(), VALUE_OF(i)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be + // CHECK-FIXES: s.count(VALUE_OF(i)); + + find(s.begin(), s.end(), PLAIN_VALUE); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be + // CHECK-FIXES: s.find(PLAIN_VALUE); + + find(s.begin(), s.end(), PLAIN_VALUE + i); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be + // CHECK-FIXES: s.find(PLAIN_VALUE + i); + + find(s.begin(), VAL_AND_END); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be + // CHECK-FIXES: find(s.begin(), VAL_AND_END); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
