[clang-tidy] Fix width/precision argument order in modernize-use-std-print

[mikecrowe]

Summary

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] https://github.com/fmtlib/fmt/pull/3515#issuecomment-1615259893

[PiotrZSL]

Just few nits, from functionally point of view looks fine.

clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp

356–357

clang-tools-extra/clang-tidy/utils/FormatStringConverter.h

73

NOTE: You can use std::pair here.

[mikecrowe]

Thanks for the review.

clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp

356–357

Good point.

clang-tools-extra/clang-tidy/utils/FormatStringConverter.h

73

True, but in my mind std::pair only exists because std::tuple couldn't be implemented in the C++98.

I was going to change it to use std::pair anyway, but I notice that I already use std::tuple for ArgFixes just above. Should I change both of them?

[mikecrowe]

Use emplace_back rather than push_back(make_tuple())

[PiotrZSL]

clang-tools-extra/clang-tidy/utils/FormatStringConverter.h

73

It will be compiled to same thing anyway. So it's up to you. Keep it consistent.
I personally use std::pair for 2 arguments, and std::tuple for more than 2.
But to be honest probably better would be to introduce some structure so that those two unsigned could be named. And probably same for ArgFixes, but there we got different types, so it's not a big problem.

[mikecrowe]

clang-tools-extra/clang-tidy/utils/FormatStringConverter.h

73

I think that you're probably right, but the names won't be used from the call site since I use structured binding there. Do you mind if I do both together in a separate commit?

[PiotrZSL]

clang-tools-extra/clang-tidy/utils/FormatStringConverter.h

73

Not a problem, lets leave it for a next change.

clang-tools-extra/clang-tidy/utils/FormatStringConverter.h

private:
 
   const StringLiteral *FormatExpr;
   std::string StandardFormatString;
 
   /// Casts to be used to wrap arguments to retain printf compatibility.
   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);
   void emitFieldWidth(const PrintfSpecifier &FS, std::string &FormatSpec);
   void emitPrecision(const PrintfSpecifier &FS, std::string &FormatSpec);
   void emitStringArgument(const Expr *Arg);
   bool emitIntegerArgument(ConversionSpecifier::Kind ArgKind, const Expr *Arg,
                            unsigned ArgIndex, std::string &FormatSpec);
 
   bool emitType(const PrintfSpecifier &FS, const Expr *Arg,
                 std::string &FormatSpec);
   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;
 
   void appendFormatText(StringRef Text);
   void finalizeFormatText();
   bool conversionNotPossible(std::string Reason) {
     ConversionNotPossibleReason = std::move(Reason);
     return false;
   }
 };
 
 } // namespace clang::tidy::utils
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_FORMATSTRINGCONVERTER_H

clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp

case OptionalAmount::Arg:
     }
     FormatSpec.push_back('}');
     break;
   case OptionalAmount::Invalid:
     break;
   }
 }
 
+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
   // pass the std::string directly. We don't want to do so if the return
   // type is not a char pointer (though it's unlikely that such code would
   // compile without warnings anyway.) See RedundantStringCStrCheck.
 
   if (!StringCStrCallExprMatcher) {
bool FormatStringConverter::convertArgument(const PrintfSpecifier &FS,
   emitSign(FS, FormatSpec);
   emitAlternativeForm(FS, FormatSpec);
 
   if (FS.hasLeadingZeros())
     FormatSpec.push_back('0');
 
   emitFieldWidth(FS, FormatSpec);
   emitPrecision(FS, FormatSpec);
+  maybeRotateArguments(FS);
 
   if (!emitType(FS, Arg, FormatSpec))
     return false;
 
   if (!FormatSpec.empty()) {
     StandardFormatString.push_back(':');
     StandardFormatString.append(FormatSpec);
   }
for (const auto &Match : ArgCStrRemovals) {
     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);
+
+    // 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

clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp

void printf_right_justified() {
   // CHECK-FIXES: std::println("Right-justified double {:4f}", 227.2);
 
   printf("Right-justified double %4g\n", 227.4);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Right-justified double {:4g}", 227.4);
 
   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]
   // CHECK-FIXES: std::println("Right-justified integer with field width argument {1:{0}} after", 5, 424242);
 
   printf("Right-justified string %20s\n", "Hello");
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Right-justified string {:>20}", "Hello");
void printf_left_justified() {
   // CHECK-FIXES: std::println("Left-justified double {:<4f}", 227.2);
 
   printf("Left-justified double %-4g\n", 227.4);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Left-justified double {:<4g}", 227.4);
 
   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]
   // CHECK-FIXES: std::println("Left-justified integer with field width argument {1:<{0}} after", 5, 424242);
 
   printf("Left-justified string %-20s\n", "Hello");
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Left-justified string {:20}", "Hello");
void printf_precision() {
   // CHECK-FIXES: std::println("Hello {:.3f}", 3.14159);
 
   printf("Hello %10.3f\n", 3.14159);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Hello {:10.3f}", 3.14159);
 
   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]
   // CHECK-FIXES: std::println("Hello {0:.{1}f} after", 3.14159265358979323846, 4);
 
   // Precision is ignored, but maintained on non-numeric arguments
   printf("Hello %.5s\n", "Goodbye");
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Hello {:.5}", "Goodbye");
 
   printf("Hello %.5c\n", 'G');
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Hello {:.5}", 'G');
 }
 
 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() {
   printf("Wibble %#x\n", 42);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("Wibble {:#x}", 42);
 
   printf("Wibble %#20x\n", 42);