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