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

[Mike Crowe via Phabricator via cfe-commits]

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

Thank you for all the review comments.



================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:322
+          if (!isRealCharType(Pointee))
+            ArgFixes.emplace_back(Arg, "reinterpret_cast<const char *>(");
+        }
----------------
njames93 wrote:
> By default this check should not do this conversion, we shouldn't be creating UB in fixes emitted by clang-tidy unless the user explicitly opts in.
> Maybe a good framework moving forward is to still a warning about converting up std::print, but emit a note here saying why the automatic conversation isn't possible.
Good spot. This was wider than I intended. The cast is only supposed to be used if the argument is a pointer to `unsigned char` or `signed char`.

I think that I probably ought to make sure that all the places that decide that conversion is not possible emit a warning explaining why.


================
Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:412
+        else
+          ArgFixes.emplace_back(Arg, "reinterpret_cast<const void *>(");
+        break;
----------------
njames93 wrote:
> `reinterpret_cast` is not needed here, a static cast to`const void*` will work.
Good spot. I'm sure I used the right cast in an earlier version... :(


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:73
+   printf-style format string and the arguments to be formatted follow
+   immediately afterwards.
+
----------------
Eugene.Zelenko wrote:
> Please add information about default value.
The current implementation always "fixes" `printf`, `fprintf`, `absl::PrintF` and `absl::FPrintf`, even when this option is used (see `UseStdPrintCheck` constructor.) Should the option inhibit this default?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:92
+
+It is helpful to use the `readability-redundant-string-cstr
+<../readability/redundant-string-cstr.html>` check after this check to
----------------
njames93 wrote:
> It may be wise in a future version to just do that conversion anyway. 2 stage fixes are annoying for users to have to use 
I shall investigate how to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149280