[clang-tools-extra] [clang-tidy] suggest fix it hints for string concat (PR #188430)
Tom Eccles via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 26 08:49:05 PDT 2026
https://github.com/tblah updated https://github.com/llvm/llvm-project/pull/188430
>From 346c20f3ed58659e4c8db9de451c7fd570e94524 Mon Sep 17 00:00:00 2001
From: Immad Mir <mirimmad17 at gmail.com>
Date: Wed, 25 Mar 2026 13:57:57 +0530
Subject: [PATCH 1/2] suggest fix it hints for string concat
---
.../InefficientStringConcatenationCheck.cpp | 169 +++++++++++++++++-
.../inefficient-string-concatenation.cpp | 27 ++-
2 files changed, 179 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
index 1067fca289a2c..ac0cdaa091f85 100644
--- a/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
@@ -8,6 +8,7 @@
#include "InefficientStringConcatenationCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
@@ -40,25 +41,101 @@ void InefficientStringConcatenationCheck::registerMatchers(
hasDescendant(BasicStringPlusOperator))
.bind("plusOperator");
+
+
+ /* const auto AssignOperator =
+ cxxOperatorCallExpr(
+ hasOverloadedOperatorName("="),
+
+ hasArgument(0, ignoringParenImpCasts(
+ declRefExpr(BasicStringType,
+ hasDeclaration(decl().bind("lhsStrT")))
+ .bind("lhsStr"))),
+
+ hasArgument(1, ignoringParenImpCasts(
+ expr(
+ hasDescendant(declRefExpr(
+ hasDeclaration(decl(equalsBoundNode("lhsStrT")))
+ )),
+ hasDescendant(BasicStringPlusOperator)
+ ).bind("rhsExpr")
+ ))
+ ).bind("assign"); */
+
+
const auto AssignOperator = cxxOperatorCallExpr(
- hasOverloadedOperatorName("="),
- hasArgument(0, declRefExpr(BasicStringType,
- hasDeclaration(decl().bind("lhsStrT")))
- .bind("lhsStr")),
- hasArgument(1, stmt(hasDescendant(declRefExpr(
- hasDeclaration(decl(equalsBoundNode("lhsStrT"))))))),
- hasDescendant(BasicStringPlusOperator));
+ hasOverloadedOperatorName("="),
+ hasArgument(0, ignoringParenImpCasts(
+ declRefExpr(BasicStringType,
+ hasDeclaration(decl().bind("lhsStrT")))
+ .bind("lhsStr"))),
+ hasArgument(1, expr(hasDescendant(BasicStringPlusOperator)).bind("rhsExpr"))
+ ).bind("assign");
+
if (StrictMode) {
Finder->addMatcher(cxxOperatorCallExpr(anyOf(AssignOperator, PlusOperator)),
- this);
+ this);
} else {
Finder->addMatcher(cxxOperatorCallExpr(anyOf(AssignOperator, PlusOperator),
hasAncestor(stmt(anyOf(
cxxForRangeStmt(), whileStmt(),
forStmt(), doStmt())))),
this);
+
+
+ }
+}
+
+static const Expr *strip(const Expr *E) {
+ while (true) {
+ E = E->IgnoreParenImpCasts();
+ if (const auto *ICE = dyn_cast<ImplicitCastExpr>(E)) {
+ E = ICE->getSubExpr();
+ }
+ else if (const auto *M = dyn_cast<MaterializeTemporaryExpr>(E))
+ {
+ E = M->getSubExpr();
+ }
+
+ else if (const auto *B = dyn_cast<CXXBindTemporaryExpr>(E)) {
+ E = B->getSubExpr();
+ }
+ else {
+ break;
+ }
}
+ return E;
+}
+
+static void collectOperands(const Expr *E, std::vector<const Expr *> &Ops) {
+ E = strip(E);
+ E = E->IgnoreParenImpCasts();
+
+ if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
+ if (BinOp->getOpcode() == BO_Add) {
+ collectOperands(BinOp->getLHS(), Ops);
+ collectOperands(BinOp->getRHS(), Ops);
+ return;
+ }
+ }
+
+
+ if (const auto *OpCall = dyn_cast<CXXOperatorCallExpr>(E)) {
+ if (OpCall->getOperator() == OO_Plus) {
+ collectOperands(OpCall->getArg(0), Ops);
+ collectOperands(OpCall->getArg(1), Ops);
+ return;
+ }
+ }
+
+ Ops.push_back(E);
+}
+
+static bool isSameLhs(const Expr *E, const DeclRefExpr *Lhs) {
+ E = strip(E)->IgnoreParenImpCasts();
+ auto *DR = dyn_cast<DeclRefExpr>(E);
+ return DR && DR->getDecl() == Lhs->getDecl();
}
void InefficientStringConcatenationCheck::check(
@@ -66,14 +143,88 @@ void InefficientStringConcatenationCheck::check(
const auto *LhsStr = Result.Nodes.getNodeAs<DeclRefExpr>("lhsStr");
const auto *PlusOperator =
Result.Nodes.getNodeAs<CXXOperatorCallExpr>("plusOperator");
+ const auto *Assign = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("assign");
+ const auto *RhsExpr = Result.Nodes.getNodeAs<Expr>("rhsExpr");
+
+ /* if (Assign) {
+ llvm::errs() << "Assign: " << Assign->getSourceRange().printToString(*Result.SourceManager) << "\n";
+ Assign->printPretty(llvm::outs(), nullptr, PrintingPolicy(Result.Context->getLangOpts()));
+ llvm::outs() << "\n";
+ }
+ if (RhsExpr) {
+ llvm::errs() << "RhsExpr: " << RhsExpr->getSourceRange().printToString(*Result.SourceManager) << "\n";
+ RhsExpr->printPretty(llvm::outs(), nullptr, PrintingPolicy(Result.Context->getLangOpts()));
+ llvm::outs() << "\n";
+ //RhsExpr->dump();
+ }
+ if (LhsStr) {
+ llvm::errs() << "LhsStr: " << LhsStr->getSourceRange().printToString(*Result.SourceManager) << "\n";
+ } */
+
+ /* std::vector<const Expr *> Operands;
+ collectOperands(RhsExpr, Operands); */
+ /* llvm::errs() << "Operands in RhsExpr:\n";
+ for (const auto *Op : Operands) {
+
+ Op->printPretty(llvm::outs(), nullptr, PrintingPolicy(Result.Context->getLangOpts()));
+ llvm::outs() << "\n";
+ } */
+
const char *DiagMsg =
"string concatenation results in allocation of unnecessary temporary "
"strings; consider using 'operator+=' or 'string::append()' instead";
- if (LhsStr)
+
+ if (Assign && LhsStr && RhsExpr) {
+ const auto &SM = *Result.SourceManager;
+ const auto &LO = Result.Context->getLangOpts();
+
+ std::vector<const Expr *> Operands;
+ collectOperands(RhsExpr, Operands);
+
+ int32_t LhsPosition = -1;
+ for (int32_t i = 0; i < Operands.size(); ++i)
+ if (isSameLhs(Operands[i], LhsStr))
+ {
+ LhsPosition = i;
+ break;
+ }
+ // skip this pattern: a => b + c + d
+ if (LhsPosition == -1) {
+ return;
+ }
+
+ auto ReplacementText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LhsStr->getSourceRange()), SM, LO).str();
+
+
+ if (Operands.size() > 2) {
+ for (int32_t i = 0; i < Operands.size(); ++i) {
+ if (i == LhsPosition)
+ continue;
+ auto OpText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Operands[i]->getSourceRange()), SM, LO);
+ ReplacementText += ".append(" + OpText.str() + ")";
+ }
+ } else {
+ auto RhsText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Operands[LhsPosition == 0 ? 1 : 0]->getSourceRange()), SM, LO);
+ ReplacementText += " += " + RhsText.str() ;
+ }
+
+
+ diag(Assign->getExprLoc(), DiagMsg)
+ << FixItHint::CreateReplacement(Assign->getSourceRange(),
+ ReplacementText);
+
+
+ } else {
+ if (LhsStr)
diag(LhsStr->getExprLoc(), DiagMsg);
else if (PlusOperator)
diag(PlusOperator->getExprLoc(), DiagMsg);
+
+ }
}
} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-string-concatenation.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-string-concatenation.cpp
index adc37e4c4bedf..15542052a5a8c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-string-concatenation.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-string-concatenation.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-inefficient-string-concatenation %t
+// RUN: %check_clang_tidy %s performance-inefficient-string-concatenation %t
#include <string>
void f(std::string) {}
@@ -14,18 +14,28 @@ int main() {
f(mystr1 + mystr2 + mystr1);
// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: string concatenation results in allocation of unnecessary temporary strings; consider using 'operator+=' or 'string::append()' instead
mystr1 = mystr1 + mystr2;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: string concatenation
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: string concatenation
+ // CHECK-FIXES: mystr1 += mystr2;
mystr1 = mystr2 + mystr2 + mystr2;
- // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: string concatenation
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: string concatenation
mystr1 = mystr2 + mystr1;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: string concatenation
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: string concatenation
+ // CHECK-FIXES: mystr1 += mystr2;
mywstr1 = mywstr2 + mywstr1;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: string concatenation
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: string concatenation
+ // CHECK-FIXES: mywstr1 += mywstr2;
mywstr1 = mywstr2 + mywstr2 + mywstr2;
// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: string concatenation
myautostr1 = myautostr1 + myautostr2;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: string concatenation
-
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: string concatenation
+ // CHECK-FIXES: myautostr1 += myautostr2;
+
+ // Match against multple lines
+ /* mystr1 = mystr1 + mystr2 + mystr2;
+ // CHECK-DAG: :33:12: warning: string concatenation
+ // CHECK-DAG: :33:30: warning: string concatenation
+ // CHECK-FIXES: mystr1.append(mystr2).append(mystr2) */
+
mywstr1 = mywstr2 + mywstr2;
mystr1 = mystr2 + mystr2;
mystr1 += mystr2;
@@ -35,7 +45,8 @@ int main() {
do {
mystr1 = mystr1 + mystr2;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: string concatenation results in allocation of unnecessary temporary strings; consider using 'operator+=' or 'string::append()' instead
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: string concatenation results in allocation of unnecessary temporary strings; consider using 'operator+=' or 'string::append()' instead
+ // CHECK-FIXES: mystr1 += mystr2;
} while (0);
return 0;
>From b563637429cc75564814f03b72934ed62c3d68bb Mon Sep 17 00:00:00 2001
From: Immad Mir <mirimmad17 at gmail.com>
Date: Wed, 25 Mar 2026 15:21:07 +0530
Subject: [PATCH 2/2] format
---
.../InefficientStringConcatenationCheck.cpp | 194 +++++++++---------
1 file changed, 93 insertions(+), 101 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
index ac0cdaa091f85..3087ddc27cb53 100644
--- a/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
@@ -41,67 +41,60 @@ void InefficientStringConcatenationCheck::registerMatchers(
hasDescendant(BasicStringPlusOperator))
.bind("plusOperator");
-
-
- /* const auto AssignOperator =
- cxxOperatorCallExpr(
- hasOverloadedOperatorName("="),
-
- hasArgument(0, ignoringParenImpCasts(
- declRefExpr(BasicStringType,
- hasDeclaration(decl().bind("lhsStrT")))
- .bind("lhsStr"))),
-
- hasArgument(1, ignoringParenImpCasts(
- expr(
- hasDescendant(declRefExpr(
- hasDeclaration(decl(equalsBoundNode("lhsStrT")))
- )),
- hasDescendant(BasicStringPlusOperator)
- ).bind("rhsExpr")
- ))
- ).bind("assign"); */
-
-
- const auto AssignOperator = cxxOperatorCallExpr(
- hasOverloadedOperatorName("="),
- hasArgument(0, ignoringParenImpCasts(
- declRefExpr(BasicStringType,
- hasDeclaration(decl().bind("lhsStrT")))
- .bind("lhsStr"))),
- hasArgument(1, expr(hasDescendant(BasicStringPlusOperator)).bind("rhsExpr"))
- ).bind("assign");
-
+ /* const auto AssignOperator =
+ cxxOperatorCallExpr(
+ hasOverloadedOperatorName("="),
+
+ hasArgument(0, ignoringParenImpCasts(
+ declRefExpr(BasicStringType,
+ hasDeclaration(decl().bind("lhsStrT")))
+ .bind("lhsStr"))),
+
+ hasArgument(1, ignoringParenImpCasts(
+ expr(
+ hasDescendant(declRefExpr(
+ hasDeclaration(decl(equalsBoundNode("lhsStrT")))
+ )),
+ hasDescendant(BasicStringPlusOperator)
+ ).bind("rhsExpr")
+ ))
+ ).bind("assign"); */
+
+ const auto AssignOperator =
+ cxxOperatorCallExpr(
+ hasOverloadedOperatorName("="),
+ hasArgument(0, ignoringParenImpCasts(
+ declRefExpr(BasicStringType,
+ hasDeclaration(decl().bind("lhsStrT")))
+ .bind("lhsStr"))),
+ hasArgument(
+ 1, expr(hasDescendant(BasicStringPlusOperator)).bind("rhsExpr")))
+ .bind("assign");
if (StrictMode) {
Finder->addMatcher(cxxOperatorCallExpr(anyOf(AssignOperator, PlusOperator)),
- this);
+ this);
} else {
Finder->addMatcher(cxxOperatorCallExpr(anyOf(AssignOperator, PlusOperator),
hasAncestor(stmt(anyOf(
cxxForRangeStmt(), whileStmt(),
forStmt(), doStmt())))),
this);
-
-
}
}
static const Expr *strip(const Expr *E) {
while (true) {
E = E->IgnoreParenImpCasts();
- if (const auto *ICE = dyn_cast<ImplicitCastExpr>(E)) {
- E = ICE->getSubExpr();
- }
- else if (const auto *M = dyn_cast<MaterializeTemporaryExpr>(E))
- {
+ if (const auto *ICE = dyn_cast<ImplicitCastExpr>(E)) {
+ E = ICE->getSubExpr();
+ } else if (const auto *M = dyn_cast<MaterializeTemporaryExpr>(E)) {
E = M->getSubExpr();
}
else if (const auto *B = dyn_cast<CXXBindTemporaryExpr>(E)) {
E = B->getSubExpr();
- }
- else {
+ } else {
break;
}
}
@@ -120,7 +113,6 @@ static void collectOperands(const Expr *E, std::vector<const Expr *> &Ops) {
}
}
-
if (const auto *OpCall = dyn_cast<CXXOperatorCallExpr>(E)) {
if (OpCall->getOperator() == OO_Plus) {
collectOperands(OpCall->getArg(0), Ops);
@@ -146,85 +138,85 @@ void InefficientStringConcatenationCheck::check(
const auto *Assign = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("assign");
const auto *RhsExpr = Result.Nodes.getNodeAs<Expr>("rhsExpr");
- /* if (Assign) {
- llvm::errs() << "Assign: " << Assign->getSourceRange().printToString(*Result.SourceManager) << "\n";
- Assign->printPretty(llvm::outs(), nullptr, PrintingPolicy(Result.Context->getLangOpts()));
- llvm::outs() << "\n";
- }
- if (RhsExpr) {
- llvm::errs() << "RhsExpr: " << RhsExpr->getSourceRange().printToString(*Result.SourceManager) << "\n";
- RhsExpr->printPretty(llvm::outs(), nullptr, PrintingPolicy(Result.Context->getLangOpts()));
- llvm::outs() << "\n";
- //RhsExpr->dump();
- }
- if (LhsStr) {
- llvm::errs() << "LhsStr: " << LhsStr->getSourceRange().printToString(*Result.SourceManager) << "\n";
- } */
-
- /* std::vector<const Expr *> Operands;
- collectOperands(RhsExpr, Operands); */
- /* llvm::errs() << "Operands in RhsExpr:\n";
- for (const auto *Op : Operands) {
-
- Op->printPretty(llvm::outs(), nullptr, PrintingPolicy(Result.Context->getLangOpts()));
- llvm::outs() << "\n";
- } */
+ /* if (Assign) {
+ llvm::errs() << "Assign: " <<
+ Assign->getSourceRange().printToString(*Result.SourceManager) << "\n";
+ Assign->printPretty(llvm::outs(), nullptr,
+ PrintingPolicy(Result.Context->getLangOpts())); llvm::outs() << "\n";
+ }
+ if (RhsExpr) {
+ llvm::errs() << "RhsExpr: " <<
+ RhsExpr->getSourceRange().printToString(*Result.SourceManager) << "\n";
+ RhsExpr->printPretty(llvm::outs(), nullptr,
+ PrintingPolicy(Result.Context->getLangOpts())); llvm::outs() << "\n";
+ //RhsExpr->dump();
+ }
+ if (LhsStr) {
+ llvm::errs() << "LhsStr: " <<
+ LhsStr->getSourceRange().printToString(*Result.SourceManager) << "\n";
+ } */
+
+ /* std::vector<const Expr *> Operands;
+ collectOperands(RhsExpr, Operands); */
+ /* llvm::errs() << "Operands in RhsExpr:\n";
+ for (const auto *Op : Operands) {
+
+ Op->printPretty(llvm::outs(), nullptr,
+ PrintingPolicy(Result.Context->getLangOpts())); llvm::outs() << "\n";
+ } */
const char *DiagMsg =
"string concatenation results in allocation of unnecessary temporary "
"strings; consider using 'operator+=' or 'string::append()' instead";
-
- if (Assign && LhsStr && RhsExpr) {
- const auto &SM = *Result.SourceManager;
- const auto &LO = Result.Context->getLangOpts();
+ if (Assign && LhsStr && RhsExpr) {
+ const auto &SM = *Result.SourceManager;
+ const auto &LO = Result.Context->getLangOpts();
std::vector<const Expr *> Operands;
collectOperands(RhsExpr, Operands);
int32_t LhsPosition = -1;
for (int32_t i = 0; i < Operands.size(); ++i)
- if (isSameLhs(Operands[i], LhsStr))
- {
- LhsPosition = i;
- break;
- }
+ if (isSameLhs(Operands[i], LhsStr)) {
+ LhsPosition = i;
+ break;
+ }
// skip this pattern: a => b + c + d
- if (LhsPosition == -1) {
+ if (LhsPosition == -1)
return;
- }
-
- auto ReplacementText = Lexer::getSourceText(
- CharSourceRange::getTokenRange(LhsStr->getSourceRange()), SM, LO).str();
+ auto ReplacementText =
+ Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LhsStr->getSourceRange()), SM, LO)
+ .str();
if (Operands.size() > 2) {
- for (int32_t i = 0; i < Operands.size(); ++i) {
- if (i == LhsPosition)
- continue;
- auto OpText = Lexer::getSourceText(
- CharSourceRange::getTokenRange(Operands[i]->getSourceRange()), SM, LO);
+ for (int32_t i = 0; i < Operands.size(); ++i) {
+ if (i == LhsPosition)
+ continue;
+ auto OpText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Operands[i]->getSourceRange()), SM,
+ LO);
ReplacementText += ".append(" + OpText.str() + ")";
+ }
+ } else {
+ auto RhsText = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(
+ Operands[LhsPosition == 0 ? 1 : 0]->getSourceRange()),
+ SM, LO);
+ ReplacementText += " += " + RhsText.str();
}
+
+ diag(Assign->getExprLoc(), DiagMsg) << FixItHint::CreateReplacement(
+ Assign->getSourceRange(), ReplacementText);
+
} else {
- auto RhsText = Lexer::getSourceText(
- CharSourceRange::getTokenRange(Operands[LhsPosition == 0 ? 1 : 0]->getSourceRange()), SM, LO);
- ReplacementText += " += " + RhsText.str() ;
+ if (LhsStr)
+ diag(LhsStr->getExprLoc(), DiagMsg);
+ else if (PlusOperator)
+ diag(PlusOperator->getExprLoc(), DiagMsg);
}
-
-
- diag(Assign->getExprLoc(), DiagMsg)
- << FixItHint::CreateReplacement(Assign->getSourceRange(),
- ReplacementText);
-
-
- } else {
- if (LhsStr)
- diag(LhsStr->getExprLoc(), DiagMsg);
- else if (PlusOperator)
- diag(PlusOperator->getExprLoc(), DiagMsg);
-
- }
}
} // namespace clang::tidy::performance
More information about the cfe-commits
mailing list