[clang-tools-extra] c6207f6 - [clang-tidy] Fix c_str() removal and cast addition when re-ordering arguments
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 28 12:44:39 PDT 2023
Author: Mike Crowe
Date: 2023-08-28T19:44:23Z
New Revision: c6207f6e0d04f129b4ec2ada644c0224038f4705
URL: https://github.com/llvm/llvm-project/commit/c6207f6e0d04f129b4ec2ada644c0224038f4705
DIFF: https://github.com/llvm/llvm-project/commit/c6207f6e0d04f129b4ec2ada644c0224038f4705.diff
LOG: [clang-tidy] Fix c_str() removal and cast addition when re-ordering arguments
The modernize-use-std-print check would get confused if it had to
re-order field-width and precision arguments at the same time as adding
casts or removing calls to c_str().
Fix this by tracking the argument indices and combining c_str() removal
with argument re-ordering. Add missing test cases to lit check.
Fixes https://github.com/llvm/llvm-project/issues/64033
Reviewed By: PiotrZSL
Differential Revision: https://reviews.llvm.org/D156616
Added:
Modified:
clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
index 951e0acf79b1ae..ad10f745b6acfb 100644
--- a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
+++ b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
@@ -356,7 +356,8 @@ void FormatStringConverter::maybeRotateArguments(const PrintfSpecifier &FS) {
ArgRotates.emplace_back(FS.getArgIndex() + ArgsOffset, ArgCount);
}
-void FormatStringConverter::emitStringArgument(const Expr *Arg) {
+void FormatStringConverter::emitStringArgument(unsigned ArgIndex,
+ const Expr *Arg) {
// If the argument is the result of a call to std::string::c_str() or
// data() with a return type of char then we can remove that call and
// pass the std::string directly. We don't want to do so if the return
@@ -386,7 +387,7 @@ void FormatStringConverter::emitStringArgument(const Expr *Arg) {
// printf is happy to print signed char and unsigned char strings, but
// std::format only likes char strings.
if (Pointee->isCharType() && !isRealCharType(Pointee))
- ArgFixes.emplace_back(Arg, "reinterpret_cast<const char *>(");
+ ArgFixes.emplace_back(ArgIndex, "reinterpret_cast<const char *>(");
}
}
@@ -409,7 +410,7 @@ bool FormatStringConverter::emitIntegerArgument(
if (const std::optional<std::string> MaybeCastType =
castTypeForArgument(ArgKind, ET->getDecl()->getIntegerType()))
ArgFixes.emplace_back(
- Arg, (Twine("static_cast<") + *MaybeCastType + ">(").str());
+ ArgIndex, (Twine("static_cast<") + *MaybeCastType + ">(").str());
else
return conversionNotPossible(
(Twine("argument ") + Twine(ArgIndex) + " has unexpected enum type")
@@ -423,7 +424,7 @@ bool FormatStringConverter::emitIntegerArgument(
if (const std::optional<std::string> MaybeCastType =
castTypeForArgument(ArgKind, ArgType))
ArgFixes.emplace_back(
- Arg, (Twine("static_cast<") + *MaybeCastType + ">(").str());
+ ArgIndex, (Twine("static_cast<") + *MaybeCastType + ">(").str());
else
return conversionNotPossible(
(Twine("argument ") + Twine(ArgIndex) + " cannot be cast to " +
@@ -447,7 +448,7 @@ bool FormatStringConverter::emitType(const PrintfSpecifier &FS, const Expr *Arg,
ConversionSpecifier::Kind ArgKind = FS.getConversionSpecifier().getKind();
switch (ArgKind) {
case ConversionSpecifier::Kind::sArg:
- emitStringArgument(Arg);
+ emitStringArgument(FS.getArgIndex() + ArgsOffset, Arg);
break;
case ConversionSpecifier::Kind::cArg:
// The type must be "c" to get a character unless the type is exactly
@@ -466,7 +467,8 @@ bool FormatStringConverter::emitType(const PrintfSpecifier &FS, const Expr *Arg,
const clang::QualType &ArgType = Arg->getType();
// std::format knows how to format void pointers and nullptrs
if (!ArgType->isNullPtrType() && !ArgType->isVoidPointerType())
- ArgFixes.emplace_back(Arg, "static_cast<const void *>(");
+ ArgFixes.emplace_back(FS.getArgIndex() + ArgsOffset,
+ "static_cast<const void *>(");
break;
}
case ConversionSpecifier::Kind::xArg:
@@ -673,6 +675,15 @@ void FormatStringConverter::appendFormatText(const StringRef Text) {
}
}
+static std::string withoutCStrReplacement(const BoundNodes &CStrRemovalMatch,
+ ASTContext &Context) {
+ const auto *Arg = CStrRemovalMatch.getNodeAs<Expr>("arg");
+ const auto *Member = CStrRemovalMatch.getNodeAs<MemberExpr>("member");
+ const bool Arrow = Member->isArrow();
+ return Arrow ? utils::fixit::formatDereference(*Arg, Context)
+ : tooling::fixit::getText(*Arg, Context).str();
+}
+
/// Called by the check when it is ready to apply the fixes.
void FormatStringConverter::applyFixes(DiagnosticBuilder &Diag,
SourceManager &SM) {
@@ -683,34 +694,35 @@ void FormatStringConverter::applyFixes(DiagnosticBuilder &Diag,
StandardFormatString);
}
- for (const auto &[Arg, Replacement] : ArgFixes) {
- SourceLocation AfterOtherSide =
- Lexer::findNextToken(Arg->getEndLoc(), SM, LangOpts)->getLocation();
-
- Diag << FixItHint::CreateInsertion(Arg->getBeginLoc(), Replacement)
- << FixItHint::CreateInsertion(AfterOtherSide, ")");
- }
-
- for (const auto &Match : ArgCStrRemovals) {
- const auto *Call = Match.getNodeAs<CallExpr>("call");
- const auto *Arg = Match.getNodeAs<Expr>("arg");
- const auto *Member = Match.getNodeAs<MemberExpr>("member");
- const bool Arrow = Member->isArrow();
- const std::string ArgText =
- Arrow ? utils::fixit::formatDereference(*Arg, *Context)
- : tooling::fixit::getText(*Arg, *Context).str();
- if (!ArgText.empty())
- Diag << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText);
- }
-
// ArgCount is one less than the number of arguments to be rotated.
for (auto [ValueArgIndex, ArgCount] : ArgRotates) {
assert(ValueArgIndex < NumArgs);
assert(ValueArgIndex > ArgCount);
- // First move the value argument to the right place.
- Diag << tooling::fixit::createReplacement(*Args[ValueArgIndex - ArgCount],
- *Args[ValueArgIndex], *Context);
+ // First move the value argument to the right place. But if there's a
+ // pending c_str() removal then we must do that at the same time.
+ if (const auto CStrRemovalMatch =
+ std::find_if(ArgCStrRemovals.cbegin(), ArgCStrRemovals.cend(),
+ [ArgStartPos = Args[ValueArgIndex]->getBeginLoc()](
+ const BoundNodes &Match) {
+ // This c_str() removal corresponds to the argument
+ // being moved if they start at the same location.
+ const Expr *CStrArg = Match.getNodeAs<Expr>("arg");
+ return ArgStartPos == CStrArg->getBeginLoc();
+ });
+ CStrRemovalMatch != ArgCStrRemovals.end()) {
+ const std::string ArgText =
+ withoutCStrReplacement(*CStrRemovalMatch, *Context);
+ assert(!ArgText.empty());
+
+ Diag << FixItHint::CreateReplacement(
+ Args[ValueArgIndex - ArgCount]->getSourceRange(), ArgText);
+
+ // That c_str() removal is now dealt with, so we don't need to do it again
+ ArgCStrRemovals.erase(CStrRemovalMatch);
+ } else
+ Diag << tooling::fixit::createReplacement(*Args[ValueArgIndex - ArgCount],
+ *Args[ValueArgIndex], *Context);
// Now shift down the field width and precision (if either are present) to
// accommodate it.
@@ -718,6 +730,31 @@ void FormatStringConverter::applyFixes(DiagnosticBuilder &Diag,
Diag << tooling::fixit::createReplacement(
*Args[ValueArgIndex - Offset], *Args[ValueArgIndex - Offset - 1],
*Context);
+
+ // Now we need to modify the ArgFix index too so that we fix the right
+ // argument. We don't need to care about the width and precision indices
+ // since they never need fixing.
+ for (auto &ArgFix : ArgFixes) {
+ if (ArgFix.ArgIndex == ValueArgIndex)
+ ArgFix.ArgIndex = ValueArgIndex - ArgCount;
+ }
+ }
+
+ for (const auto &[ArgIndex, Replacement] : ArgFixes) {
+ SourceLocation AfterOtherSide =
+ Lexer::findNextToken(Args[ArgIndex]->getEndLoc(), SM, LangOpts)
+ ->getLocation();
+
+ Diag << FixItHint::CreateInsertion(Args[ArgIndex]->getBeginLoc(),
+ Replacement, true)
+ << FixItHint::CreateInsertion(AfterOtherSide, ")", true);
+ }
+
+ for (const auto &Match : ArgCStrRemovals) {
+ const auto *Call = Match.getNodeAs<CallExpr>("call");
+ const std::string ArgText = withoutCStrReplacement(Match, *Context);
+ if (!ArgText.empty())
+ Diag << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText);
}
}
} // namespace clang::tidy::utils
diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
index 200e530b40810b..1949870f62ed68 100644
--- a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
+++ b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
@@ -64,7 +64,16 @@ class FormatStringConverter
std::string StandardFormatString;
/// Casts to be used to wrap arguments to retain printf compatibility.
- std::vector<std::tuple<const Expr *, std::string>> ArgFixes;
+ struct ArgumentFix {
+ unsigned ArgIndex;
+ std::string Fix;
+
+ // We currently need this for emplace_back. Roll on C++20.
+ explicit ArgumentFix(unsigned ArgIndex, std::string Fix)
+ : ArgIndex(ArgIndex), Fix(std::move(Fix)) {}
+ };
+
+ std::vector<ArgumentFix> ArgFixes;
std::vector<clang::ast_matchers::BoundNodes> ArgCStrRemovals;
// Argument rotations to cope with the fact that std::print puts the value to
@@ -77,7 +86,7 @@ class FormatStringConverter
void emitAlternativeForm(const PrintfSpecifier &FS, std::string &FormatSpec);
void emitFieldWidth(const PrintfSpecifier &FS, std::string &FormatSpec);
void emitPrecision(const PrintfSpecifier &FS, std::string &FormatSpec);
- void emitStringArgument(const Expr *Arg);
+ void emitStringArgument(unsigned ArgIndex, const Expr *Arg);
bool emitIntegerArgument(ConversionSpecifier::Kind ArgKind, const Expr *Arg,
unsigned ArgIndex, std::string &FormatSpec);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
index 30a63a6b867798..da1a18782c9bed 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
@@ -14,6 +14,12 @@
#include <string.h>
#include <string>
+template <typename T>
+struct iterator {
+ T *operator->();
+ T &operator*();
+};
+
void printf_simple() {
printf("Hello");
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
@@ -1121,11 +1127,32 @@ void printf_precision() {
// CHECK-FIXES: std::println("Hello {:.5}", 'G');
}
-void printf_field_width_and_precision() {
+void printf_field_width_and_precision(const std::string &s1, const std::string &s2, const std::string &s3)
+{
printf("width only:%*d width and precision:%*.*f precision only:%.*f\n", 3, 42, 4, 2, 3.14159265358979323846, 5, 2.718);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("width only:{:{}} width and precision:{:{}.{}f} precision only:{:.{}f}", 42, 3, 3.14159265358979323846, 4, 2, 2.718, 5);
+ const unsigned int ui1 = 42, ui2 = 43, ui3 = 44;
+ printf("casts width only:%*d width and precision:%*.*d precision only:%.*d\n", 3, ui1, 4, 2, ui2, 5, ui3);
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+ // CHECK-FIXES-NOTSTRICT: std::println("casts width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", ui1, 3, ui2, 4, 2, ui3, 5);
+ // CHECK-FIXES-STRICT: std::println("casts width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", static_cast<int>(ui1), 3, static_cast<int>(ui2), 4, 2, static_cast<int>(ui3), 5);
+
+ printf("c_str removal width only:%*s width and precision:%*.*s precision only:%.*s\n", 3, s1.c_str(), 4, 2, s2.c_str(), 5, s3.c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println("c_str removal width only:{:>{}} width and precision:{:>{}.{}} precision only:{:.{}}", s1, 3, s2, 4, 2, s3, 5);
+
+ const std::string *ps1 = &s1, *ps2 = &s2, *ps3 = &s3;
+ printf("c_str() removal pointer width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, ps1->c_str(), 4, 2, ps2->c_str(), 5, ps3->c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println("c_str() removal pointer width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *ps1, 3, *ps2, 4, 2, *ps3, 5);
+
+ iterator<std::string> is1, is2, is3;
+ printf("c_str() removal iterator width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, is1->c_str(), 4, 2, is2->c_str(), 5, is3->c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println("c_str() removal iterator width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *is1, 3, *is2, 4, 2, *is3, 5);
+
printf("width and precision positional:%1$*2$.*3$f after\n", 3.14159265358979323846, 4, 2);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("width and precision positional:{0:{1}.{2}f} after", 3.14159265358979323846, 4, 2);
@@ -1134,9 +1161,13 @@ void printf_field_width_and_precision() {
printf("width only:%3$*1$d width and precision:%4$*1$.*2$f precision only:%5$.*2$f\n", width, precision, 42, 3.1415926, 2.718);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("width only:{2:{0}} width and precision:{3:{0}.{1}f} precision only:{4:.{1}f}", width, precision, 42, 3.1415926, 2.718);
+
+ printf("c_str removal width only:%3$*1$s width and precision:%4$*1$.*2$s precision only:%5$.*2$s\n", width, precision, s1.c_str(), s2.c_str(), s3.c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println("c_str removal width only:{2:>{0}} width and precision:{3:>{0}.{1}} precision only:{4:.{1}}", width, precision, s1, s2, s3);
}
-void fprintf_field_width_and_precision() {
+void fprintf_field_width_and_precision(const std::string &s1, const std::string &s2, const std::string &s3) {
fprintf(stderr, "width only:%*d width and precision:%*.*f precision only:%.*f\n", 3, 42, 4, 2, 3.14159265358979323846, 5, 2.718);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
// CHECK-FIXES: std::println(stderr, "width only:{:{}} width and precision:{:{}.{}f} precision only:{:.{}f}", 42, 3, 3.14159265358979323846, 4, 2, 2.718, 5);
@@ -1145,10 +1176,28 @@ void fprintf_field_width_and_precision() {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
// CHECK-FIXES: std::println(stderr, "width and precision positional:{0:{1}.{2}f} after", 3.14159265358979323846, 4, 2);
+ fprintf(stderr, "c_str removal width only:%*s width and precision:%*.*s precision only:%.*s\n", 3, s1.c_str(), 4, 2, s2.c_str(), 5, s3.c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println(stderr, "c_str removal width only:{:>{}} width and precision:{:>{}.{}} precision only:{:.{}}", s1, 3, s2, 4, 2, s3, 5);
+
+ const std::string *ps1 = &s1, *ps2 = &s2, *ps3 = &s3;
+ fprintf(stderr, "c_str() removal pointer width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, ps1->c_str(), 4, 2, ps2->c_str(), 5, ps3->c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println(stderr, "c_str() removal pointer width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *ps1, 3, *ps2, 4, 2, *ps3, 5);
+
+ iterator<std::string> is1, is2, is3;
+ fprintf(stderr, "c_str() removal iterator width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, is1->c_str(), 4, 2, is2->c_str(), 5, is3->c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println(stderr, "c_str() removal iterator width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *is1, 3, *is2, 4, 2, *is3, 5);
+
const int width = 10, precision = 3;
fprintf(stderr, "width only:%3$*1$d width and precision:%4$*1$.*2$f precision only:%5$.*2$f\n", width, precision, 42, 3.1415926, 2.718);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
// CHECK-FIXES: std::println(stderr, "width only:{2:{0}} width and precision:{3:{0}.{1}f} precision only:{4:.{1}f}", width, precision, 42, 3.1415926, 2.718);
+
+ fprintf(stderr, "c_str removal width only:%3$*1$s width and precision:%4$*1$.*2$s precision only:%5$.*2$s\n", width, precision, s1.c_str(), s2.c_str(), s3.c_str());
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+ // CHECK-FIXES: std::println(stderr, "c_str removal width only:{2:>{0}} width and precision:{3:>{0}.{1}} precision only:{4:.{1}}", width, precision, s1, s2, s3);
}
void printf_alternative_form() {
@@ -1497,12 +1546,6 @@ void fprintf_string_pointer_cstr(const std::string *s1) {
// CHECK-FIXES: std::print(stderr, "fprintf string pointer c_str {}", *s1);
}
-template <typename T>
-struct iterator {
- T *operator->();
- T &operator*();
-};
-
void printf_iterator_cstr(iterator<std::string> i1, iterator<std::string> i2)
{
printf("printf iterator c_str %s %s\n", i1->c_str(), i2->data());
More information about the cfe-commits
mailing list