[PATCH] D149280: [clang-tidy] Add modernize-printf-to-std-print check

[Mike Crowe via Phabricator via cfe-commits]

mikecrowe marked 2 inline comments as done.
mikecrowe added a comment.

Thanks for the further reviews.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:37
+
+  if (PrintfLikeFunctions.empty() && FprintfLikeFunctions.empty()) {
+    PrintfLikeFunctions.push_back("::printf");
----------------
PiotrZSL wrote:
> those 2 looks in-depended. Cannot they be put as default value into Option config ?
My expectation was that if you didn't want to replace `printf` (and instead wanted to replace some other `printf`-like function) then you'd also not want to replace `fprintf` and vice-versa. In my head the two options do go together. If setting one didn't also remove the default for the other you'd end up having to specify `PrintfLikeFunctions=my_printf, FPrintfLikeFunctions=` just to replace a single function.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:79
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(
+                          matchers::matchesAnyListedName(FprintfLikeFunctions))
----------------
PiotrZSL wrote:
> this could match also methods, maybe something `unless(cxxMethodDecl())` ?
I would like to be able to match methods. The code I wrote this check to run on has classes that have methods that used to call `std::fprintf`, and currently call `fmt::fprintf`. I wish to convert those methods to use `fmt::print` and use this check to fix all their callers. Eventually I hope to be able to move to `std::print`.

However, the check doesn't currently seem to work for methods since the arguments are off-by-one and the replacement is incorrect too. I'll add the `unless(cxxMethodDecl)` checks for now and support can be added later.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:82
+                          .bind("func_decl")),
+               hasArgument(1, stringLiteral()))
+          .bind("fprintf"),
----------------
PiotrZSL wrote:
> consider moving this stringLiteral to be first, consider adding something like `unless(argumentCountIs(0)), unless(argumentCountIs(1))`` at begining or create matcher that verify first that argument count is >= 2, so we could exlude most of callExpr at begining, even before we match functions
I would have hoped that `hasArgument` would fail quite fast if the argument index passed is greater than the number of arguments. I will add `argumentCountAtLeast` and use it regardless though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149280/new/

https://reviews.llvm.org/D149280