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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
