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

[Mike Crowe via Phabricator via cfe-commits]

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

Thanks for the comprehensive review.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:43-44
+
+  if (!MaybeHeaderToInclude && (ReplacementPrintFunction == "std::print" ||
+                                ReplacementPrintlnFunction == "std::println"))
+    MaybeHeaderToInclude = "<print>";
----------------
PiotrZSL wrote:
> this code is already duplicated, move it to some separate private function, like isCpp23PrintConfigured or something...
You're correct that `ReplacementPrintFunction == "std::print" || ReplacementPrintlnFunction == "std::println"` is duplicated in `isLanguageVersionSupported`, but in that code we're considering which C++ version the functions require and in this code we're considering which header the functions require. These are not the same question. It just so happens that these checks are against the same function names now, but they need not be. If another function is added to `<print>` in C++26 and this code ends up being able to convert to it then the checks would need to diverge.

Nevertheless, I will do as you request if you still think it's worthwhile.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:56
+  Finder->addMatcher(
+      traverse(TK_IgnoreUnlessSpelledInSource,
+               callExpr(callee(functionDecl(matchers::matchesAnyListedName(
----------------
PiotrZSL wrote:
> Those TK_IgnoreUnlessSpelledInSource should be moved into getCheckTraversalKind()
Ah, that was added since I wrote the original version of this check. I'll address this.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h:40
+  StringRef ReplacementPrintlnFunction;
+  utils::IncludeInserter IncludeInserter;
+  std::optional<StringRef> MaybeHeaderToInclude;
----------------
PiotrZSL wrote:
> I heard that using clang::tooling::HeaderIncludes is more recommended.
> And this is just some house-made stuff
I'm afraid that I don't really understand this comment. The use of `IncludeInserter` is far more common in the existing checks than `HeaderIncludes` and from looking at `IncludeCleaner.cpp` the use of the latter is rather more complex. New features were being added to `IncludeInserter` within the last six months. I'm unsure what "house-made" means in the context of the `IncludeInserter` code that I didn't write.

Do you have anything more concrete than hearsay? If there is a plan to deprecate `IncludeInserter` then it feels like something closer to its convenient interface would need to be implemented in terms of `HeaderIncludes` for checks to use rather than them duplicating the code in `IncludeCleaner.cpp`.

If you can explain what you'd like me to do, and ideally provide pointers to similar things being done elsewhere then perhaps I can address this comment.


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:185
+  assert(FormatExpr);
+  PrintfFormatString = FormatExpr->getString();
+
----------------
PiotrZSL wrote:
> This may crash for wchar, maybe better limit StringLiteral to Ordinary and UTF8 or only Ordinary 
It doesn't look like there's any support for u8 literals in `printf` or `std::print` (yet), so I think just Ordinary is sufficient.


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:202
+/// Called for each format specifier by ParsePrintfString.
+bool FormatStringConverter::HandlePrintfSpecifier(
+    const analyze_printf::PrintfSpecifier &FS, const char *StartSpecifier,
----------------
PiotrZSL wrote:
> this function should be split into smaller one.
I agree. I'm surprised it hasn't come up in review earlier. I tried to do so prior to pushing the very first version of this for review and gave up since the parts ended up referring to so many local variables making the code even harder to understand than it is now. I will have another attempt.


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:230
+    return conversionNotPossible("'%m' is not supported in format string");
+  } else {
+    StandardFormatString.push_back('{');
----------------
PiotrZSL wrote:
> general: no need for else after return
In general, you're correct that I've used else after return in other places and I will fix them. However, in this case the else is required since the very first `if` block above doesn't have a return. (Hopefully this complexity will go away if I succeed in splitting this function up.)


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:384-397
+        const auto StringDecl = type(hasUnqualifiedDesugaredType(recordType(
+            hasDeclaration(cxxRecordDecl(hasName("::std::basic_string"))))));
+        const auto StringExpr = expr(anyOf(
+            hasType(StringDecl), hasType(qualType(pointsTo(StringDecl)))));
+
+        const auto StringCStrCallExpr =
+            cxxMemberCallExpr(on(StringExpr.bind("arg")),
----------------
PiotrZSL wrote:
> constant construction of those marchers may be costly.. can be cache them somehow in this object ?
Good idea. Do you have any pointers to code that does this? I couldn't find any.

The types involved all seem to be in the `internal::` namespace and presumably subject to change. Ideally, I'd put:
```
std::optional<SomeSortOfMatcherType> StringCStrCallExprMatcher;
```
in the class and then populate it here the first time lazily. Unfortunately I have no idea what type to use for `SomeSortOfMatcherType`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280