diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp index 951e0acf79b1a..ad10f745b6acf 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("); + ArgFixes.emplace_back(ArgIndex, "reinterpret_cast("); } } @@ -409,7 +410,7 @@ bool FormatStringConverter::emitIntegerArgument( if (const std::optional 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 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("); + ArgFixes.emplace_back(FS.getArgIndex() + ArgsOffset, + "static_cast("); 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("arg"); + const auto *Member = CStrRemovalMatch.getNodeAs("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("call"); - const auto *Arg = Match.getNodeAs("arg"); - const auto *Member = Match.getNodeAs("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("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("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 200e530b40810..1949870f62ed6 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> 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 ArgFixes; std::vector 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 30a63a6b86779..da1a18782c9be 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 #include +template +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(ui1), 3, static_cast(ui2), 4, 2, static_cast(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 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 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 -struct iterator { - T *operator->(); - T &operator*(); -}; - void printf_iterator_cstr(iterator i1, iterator i2) { printf("printf iterator c_str %s %s\n", i1->c_str(), i2->data());