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

PiotrZSL added a comment. In D149280#4450418 , @mikecrowe wrote: > Despite the fact that I've worked on what became this check for about two > years, now that it's landed I've suddenly spotted a significant flaw: > `printf` returns the number of

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

mikecrowe added a comment. Despite the fact that I've worked on what became this check for about two years, now that it's landed I've suddenly spotted a significant flaw: `printf` returns the number of characters printed, whereas `std::print` doesn't return anything. None of my test cases made

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

mikecrowe added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:169 + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print] + // CHECK-FIXES: std::println("Integer {:d}

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

mikecrowe marked an inline comment as done. mikecrowe added a comment. In D149280#4448158 , @PiotrZSL wrote: > Test is failing: > https://lab.llvm.org/buildbot/#/builders/230/builds/14939/steps/6/logs/FAIL__Clang_Tools__use-std-print_cpp >

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

PiotrZSL added a comment. Test is failing: https://lab.llvm.org/buildbot/#/builders/230/builds/14939/steps/6/logs/FAIL__Clang_Tools__use-std-print_cpp https://lab.llvm.org/buildbot/#/builders/245/builds/10266 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

PiotrZSL added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cstddef:12 -typedef __typeof__(sizeof(0)) size_t; +#include "stddef.h" We shouldn't include this stddef.h, it's being included from system headers.

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

thakis added a comment. None i know of, but given the bot's already red, you won't make it worse :) Tests shouldn't include any headers generally, not even inttypes.h (unless the test tests that header). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

mikecrowe marked an inline comment as done. mikecrowe added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:9-15 +#include +#include +#include +// CHECK-FIXES: #include +#include +#include +#include

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

mikecrowe added a comment. In D149280#4447363 , @thakis wrote: > This breaks tests on windows: http://45.33.8.238/win/80283/step_8.txt > > Please take a look and revert for now if it takes a while to fix. Thanks for letting me know, I thought I could

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

PiotrZSL reopened this revision. PiotrZSL added a comment. This revision is now accepted and ready to land. @thakis Thank you. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:9-15 +#include +#include +#include +// CHECK-FIXES: #include

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

thakis added a comment. This breaks tests on windows: http://45.33.8.238/win/80283/step_8.txt Please take a look and revert for now if it takes a while to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149280/new/

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

mikecrowe added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:64 + if (MaybeHeaderToInclude) +Options.store(Opts, "PrintHeader", *MaybeHeaderToInclude); +} This is going to write the default value set in the

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

PiotrZSL accepted this revision. PiotrZSL added a comment. Consider delivering this check. I do not think that it will become much better with more refactoring. Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:202-203 + assert(FormatExpr); + if

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

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()) { +

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

PiotrZSL accepted this revision. PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:37 + + if (PrintfLikeFunctions.empty() && FprintfLikeFunctions.empty()) { +PrintfLikeFunctions.push_back("::printf");

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

Eugene.Zelenko added a comment. In D149280#4446591 , @mikecrowe wrote: > @Eugene.Zelenko, > I will make the documentation changes you've suggested, but I can't say I > really understand which document elements require single backticks and which >

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

mikecrowe added a comment. @Eugene.Zelenko, I will make the documentation changes you've suggested, but I can't say I really understand which document elements require single backticks and which require dual backticks. https://llvm.org/docs/SphinxQuickstartTemplate.html seems to imply that

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:29 +- It assumes that the format string is correct for the arguments. If you + get any warnings when compiling with ``-Wformat`` then misbehaviour is +

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

mikecrowe marked 4 inline comments as done. mikecrowe added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:384-397 +const auto StringDecl = type(hasUnqualifiedDesugaredType(recordType( +

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

PiotrZSL added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:117 + +.. option:: PrintFunction + mikecrowe wrote: > PiotrZSL wrote: > > mikecrowe wrote: > > > Is `PrintFunction` (and the soon-to-arrive

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

mikecrowe marked 7 inline comments as done. mikecrowe added a comment. I'm still working on the remaining items. Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:230 +return conversionNotPossible("'%m' is not supported in format string"); + } else

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

mikecrowe added inline comments. 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 , const char

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

PiotrZSL accepted this revision. PiotrZSL added a comment. This revision is now accepted and ready to land. Except readability issues with this check code (bunch of else, ..), and that I didn't took a deep dive into format string conversion (I hope that author is right there). I would say that

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

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" || +

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

PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp:43-44 + + if (!MaybeHeaderToInclude && (ReplacementPrintFunction == "std::print" || +ReplacementPrintlnFunction == "std::println")) +

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

mikecrowe added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:79 + +.. option:: StrictMode + mikecrowe wrote: > It turns out that absl::PrintF and absl::FPrintF work like std::format, > fmt::printf, etc. and

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

mikecrowe added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:79 + +.. option:: StrictMode + It turns out that absl::PrintF and absl::FPrintF work like std::format, fmt::printf, etc. and use the signedness of

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

mikecrowe added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp:27 + using namespace clang; + if (const auto *BT = llvm::dyn_cast(Ty)) { +const bool result = (BT->getKind() == BuiltinType::Char_U || This apparently

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

mikecrowe added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst:117 + +.. option:: PrintFunction + Is `PrintFunction` (and the soon-to-arrive `PrintlnFunction`) distinct enough from `PrintfLikeFunctions` and

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

mikecrowe updated this revision to Diff 520439. mikecrowe added a comment. Address many more review comments, including: - Only add casts for signed/unsigned discrepancy if StrictMode option is set. - Use IncludeInserter to add or other required header. Review comments still outstanding: -

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

mikecrowe marked 3 inline comments as done. mikecrowe added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp:124-126 + printf("Integer %d from bool\n", b); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead

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

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h:20 +if (ReplacementPrintFunction == "std::print") + return LangOpts.CPlusPlus2b; +return LangOpts.CPlusPlus; This will need rebasing and changing

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

mikecrowe updated this revision to Diff 518315. mikecrowe added a comment. I've addressed most of the review comments. The work that remains outstanding is: - Emit warnings when conversion is not possible explaining why. - Only add signed/unsigned casts when in StrictMode. - Automatically

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

mikecrowe added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert.cpp:289 + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print' [modernize-printf-to-std-print] + // CHECK-FIXES:

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

mikecrowe marked an inline comment as done. mikecrowe added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/printf-to-std-print-convert.cpp:289 + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: Replace 'printf' with 'std::print'

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

mikecrowe marked 4 inline comments as done. mikecrowe added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/printf-to-std-print.rst:61 +- any arguments where the format string and the parameter differ in + signedness will be wrapped in an

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

Eugene.Zelenko added inline comments. 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. + mikecrowe wrote: >

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

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,

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

mikecrowe added a comment. Thanks for all the reviews. In D149280#4301188 , @njames93 wrote: > It may be a good idea to create unittests for the formatconverter in > `clang-tools-extra/unittests/clang-tidy/` I started there but had decided that the

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp:66 + if (Converter.canApply()) { +const auto *PrintfCall = Printf->getCallee(); +DiagnosticBuilder Diag = Please don't use `auto` unless

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

PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/PrintfToStdPrintCheck.cpp:28 + ReplacementPrintFunction(Options.get("PrintFunction", "std::print")) { + PrintfLikeFunctions.push_back("printf"); +

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

njames93 added a comment. It may be a good idea to create unittests for the formatconverter in `clang-tools-extra/unittests/clang-tidy/` Can this check be renamed to just `modernize-use-std-print`. This sticks with the convention of other modernize names and potentially in the future allows us

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

mikecrowe added a comment. If this sort of check is deemed acceptable then I have plans for future improvements, including: 1. Automatically turning absl::StrFormat into fmt::format too (with an option to choose an alternative to `absl::StrFormat`.) 2. Detecting format strings that end in

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

mikecrowe created this revision. Herald added subscribers: carlosgalvezp, xazax.hun. Herald added a reviewer: njames93. Herald added a project: All. mikecrowe requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Add