Skip to content

Commit

Permalink
[clang-tidy] Fix width/precision argument order in modernize-use-std-…
Browse files Browse the repository at this point in the history 
…print

Victor Zverovich pointed out[1] that printf takes the field width and
precision arguments before the value to be printed whereas std::print
takes the value first (unless positional arguments are used.) Many of
the test cases in use-std-print.cpp were incorrect.

Teach the check to rotate the arguments when required to correct
this. Correct the test cases and add more.

[1] fmtlib/fmt#3515 (comment)

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D154283
  • Loading branch information
@mikecrowe @PiotrZSL
mikecrowe authored and PiotrZSL committed Jul 3, 2023
1 parent a2ff292 commit 2806cf4
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 7 deletions.
34 changes: 34 additions & 0 deletions clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,22 @@ void FormatStringConverter::emitPrecision(const PrintfSpecifier &FS,
}
}

void FormatStringConverter::maybeRotateArguments(const PrintfSpecifier &FS) {
unsigned ArgCount = 0;
const OptionalAmount FieldWidth = FS.getFieldWidth();
const OptionalAmount FieldPrecision = FS.getPrecision();

if (FieldWidth.getHowSpecified() == OptionalAmount::Arg &&
!FieldWidth.usesPositionalArg())
++ArgCount;
if (FieldPrecision.getHowSpecified() == OptionalAmount::Arg &&
!FieldPrecision.usesPositionalArg())
++ArgCount;

if (ArgCount)
ArgRotates.emplace_back(FS.getArgIndex() + ArgsOffset, ArgCount);
}

void FormatStringConverter::emitStringArgument(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
Expand Down Expand Up @@ -531,6 +547,7 @@ bool FormatStringConverter::convertArgument(const PrintfSpecifier &FS,

emitFieldWidth(FS, FormatSpec);
emitPrecision(FS, FormatSpec);
maybeRotateArguments(FS);

if (!emitType(FS, Arg, FormatSpec))
return false;
Expand Down Expand Up @@ -682,5 +699,22 @@ void FormatStringConverter::applyFixes(DiagnosticBuilder &Diag,
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);

// Now shift down the field width and precision (if either are present) to
// accommodate it.
for (size_t Offset = 0; Offset < ArgCount; ++Offset)
Diag << tooling::fixit::createReplacement(
*Args[ValueArgIndex - Offset], *Args[ValueArgIndex - Offset - 1],
*Context);
}
}
} // namespace clang::tidy::utils
7 changes: 7 additions & 0 deletions clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ class FormatStringConverter
std::vector<std::tuple<const Expr *, std::string>> ArgFixes;
std::vector<clang::ast_matchers::BoundNodes> ArgCStrRemovals;

// Argument rotations to cope with the fact that std::print puts the value to
// be formatted first and the width and precision afterwards whereas printf
// puts the width and preicision first.
std::vector<std::tuple<unsigned, unsigned>> ArgRotates;

void emitAlignment(const PrintfSpecifier &FS, std::string &FormatSpec);
void emitSign(const PrintfSpecifier &FS, std::string &FormatSpec);
void emitAlternativeForm(const PrintfSpecifier &FS, std::string &FormatSpec);
Expand All @@ -81,6 +86,8 @@ class FormatStringConverter
bool convertArgument(const PrintfSpecifier &FS, const Expr *Arg,
std::string &StandardFormatString);

void maybeRotateArguments(const PrintfSpecifier &FS);

bool HandlePrintfSpecifier(const PrintfSpecifier &FS,
const char *StartSpecifier, unsigned SpecifierLen,
const TargetInfo &Target) override;
Expand Down
38 changes: 31 additions & 7 deletions clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ void printf_right_justified() {

printf("Right-justified integer with field width argument %*d after\n", 5, 424242);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("Right-justified integer with field width argument {:{}} after", 5, 424242);
// CHECK-FIXES: std::println("Right-justified integer with field width argument {:{}} after", 424242, 5);

printf("Right-justified integer with field width argument %2$*1$d after\n", 5, 424242);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
Expand Down Expand Up @@ -1061,7 +1061,7 @@ void printf_left_justified() {

printf("Left-justified integer with field width argument %-*d after\n", 5, 424242);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("Left-justified integer with field width argument {:<{}} after", 5, 424242);
// CHECK-FIXES: std::println("Left-justified integer with field width argument {:<{}} after", 424242, 5);

printf("Left-justified integer with field width argument %2$-*1$d after\n", 5, 424242);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
Expand All @@ -1087,15 +1087,15 @@ void printf_precision() {

printf("Hello %.*f after\n", 10, 3.14159265358979323846);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("Hello {:.{}f} after", 10, 3.14159265358979323846);
// CHECK-FIXES: std::println("Hello {:.{}f} after", 3.14159265358979323846, 10);

printf("Hello %10.*f after\n", 3, 3.14159265358979323846);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("Hello {:10.{}f} after", 3, 3.14159265358979323846);
// CHECK-FIXES: std::println("Hello {:10.{}f} after", 3.14159265358979323846, 3);

printf("Hello %*.*f after\n", 10, 4, 3.14159265358979323846);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
// CHECK-FIXES: std::println("Hello {:{}.{}f} after", 10, 4, 3.14159265358979323846);
// CHECK-FIXES: std::println("Hello {:{}.{}f} after", 3.14159265358979323846, 10, 4);

printf("Hello %1$.*2$f after\n", 3.14159265358979323846, 4);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
Expand All @@ -1112,9 +1112,33 @@ void printf_precision() {
}

void printf_field_width_and_precision() {
printf("Hello %1$*2$.*3$f after\n", 3.14159265358979323846, 4, 2);
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("Hello {0:{1}.{2}f} after", 3.14159265358979323846, 4, 2);
// CHECK-FIXES: std::println("width only:{:{}} width and precision:{:{}.{}f} precision only:{:.{}f}", 42, 3, 3.14159265358979323846, 4, 2, 2.718, 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);

const int width = 10, precision = 3;
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);
}

void fprintf_field_width_and_precision() {
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);

fprintf(stderr, "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 'fprintf' [modernize-use-std-print]
// CHECK-FIXES: std::println(stderr, "width and precision positional:{0:{1}.{2}f} after", 3.14159265358979323846, 4, 2);

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);
}

void printf_alternative_form() {
Expand Down

0 comments on commit 2806cf4

Please sign in to comment.