Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[clang-tidy] Add modernize-printf-to-std-print check
ClosedPublic

Authored by mikecrowe on Apr 26 2023, 12:09 PM.

Details

Summary

Add FormatStringConverter utility class that is capable of converting
printf-style format strings into std::print-style format strings along
with recording a set of casts to wrap the arguments as required and
removing now-unnecessary calls to std::string::c_str() and
std::string::data()

Use FormatStringConverter to implement a new clang-tidy check that is
capable of converting calls to printf, fprintf, absl::PrintF,
absl::FPrintF, or any functions configured by an option to calls to
std::print and std::println, or other functions configured by options.

In other words, the check turns:

fprintf(stderr, "The %s is %3d\n", description.c_str(), value);

into:

std::println(stderr, "The {} is {:3}", description, value);

if it can.

std::print and std::println can do almost anything that standard printf
can, but the conversion has some some limitations that are described in
the documentation. If conversion is not possible then the call remains
unchanged.

Depends on D153716

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mikecrowe added inline comments.Jun 18 2023, 1:53 PM
clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
203

It turns out that things are working out much better in my second attempt at splitting this function. I shall address the other new review comments before sending another revision for review though.

mikecrowe marked 7 inline comments as done.Jun 24 2023, 7:03 AM

I'm still working on the remaining items.

clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
231

I've now reworked this function extensively, so there's no longer an else here.

clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst
118

I assume that you mean ReplacementPrintFunction. (I see ReplacementString used in other checks, so I think that just Replacement might be a bit vague.)

PiotrZSL added inline comments.Jun 24 2023, 7:15 AM
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst
118

Yes, ... ReplacementPrintFunction

mikecrowe marked 4 inline comments as done.Jun 24 2023, 8:22 AM
mikecrowe added inline comments.
clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
385–398

Thanks for the pointer. It turns out that I need just clang::ast_matchers::StatementMatcher.

mikecrowe updated this revision to Diff 534221.Jun 24 2023, 8:33 AM
mikecrowe marked an inline comment as done.

Address review comments from PiotrZSL

  • Name options ReplacementPrintFunction and ReplacementPrintlnFunction
  • Use separate function not lambda in FormatStringConverter constructor
  • Put isRealChar matcher outside clang::ast_matchers
  • Remove anonymous namespace as suggested by PiotrZSL
  • Split HandlePrintfSpecifier body into separate functions
  • Make ArgKind a constant rather than calling Spec.getKind() every time
  • Other simplifications, method renaming, comments and cleanups
mikecrowe edited the summary of this revision. (Show Details)Jun 24 2023, 8:42 AM
mikecrowe edited the summary of this revision. (Show Details)
Eugene.Zelenko added inline comments.Jun 24 2023, 9:02 AM
clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst
30

Please use single back-ticks for -Wformat (command-line option).

82

Please highlight true with back-ticks.

88

Please use single back-ticks for StrictMode (option).

104

Please highlight false and numbers with back-ticks.

113

Please use single back-ticks for FprintfLikeFunctions (option). Same for printf; absl::PrintF below.

123

Ditto.

145

Please use single back-ticks for ReplacementPrintFunction (option).

@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 single backticks are only used for links and double backticks are used for monospace, but it's not completely clear. Most of the existing documentation seems to be consistent with what you've suggested though.

mikecrowe marked 7 inline comments as done.Jun 24 2023, 9:28 AM
mikecrowe updated this revision to Diff 534227.Jun 24 2023, 9:29 AM
mikecrowe edited the summary of this revision. (Show Details)

Apply documentation backtick changes suggested by Eugene.Zelenko

@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 single backticks are only used for links and double backticks are used for monospace, but it's not completely clear. Most of the existing documentation seems to be consistent with what you've suggested though.

In short: double back-ticks are for language constructs. Single - for command-line options, options and their values.

PiotrZSL accepted this revision.Jun 24 2023, 9:53 AM
PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
38

those 2 looks in-depended. Cannot they be put as default value into Option config ?

71

call this function only if PrintfLikeFunctions is not empty.

79

call this function only if FprintfLikeFunctions is not empty

80

this could match also methods, maybe something unless(cxxMethodDecl()) ?

83

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

clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst
59
60
61
62
mikecrowe marked 2 inline comments as done.Jun 24 2023, 2:00 PM

Thanks for the further reviews.

clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
38

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.

80

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.

83

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.

Address further review comments from PiotrZSL

  • Only add matchers if they are required.
  • Check the number of arguments right at the start of the matcher using the new argumentCountAtLeast matcher added in D153716.
  • Apply further documentation backtick changes.
mikecrowe edited the summary of this revision. (Show Details)Jun 24 2023, 11:51 PM
mikecrowe marked 2 inline comments as done.
PiotrZSL accepted this revision.Jun 25 2023, 8:56 AM

Consider delivering this check. I do not think that it will become much better with more refactoring.

clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
203–204

this check for isOrdinary could be done on matcher level, just add anonymous matcher, and use it there, you can still use it also here, but my idea is to reduce amount of calls to check method.

AST_MATCHER(StringLiteral, isOrdinary) {
  return Node.isOrdinary();
}
mikecrowe updated this revision to Diff 534354.Jun 25 2023, 9:23 AM

Include isOrdinary in check as suggested by @PiotrZSL.

Thanks for all the reviews.

@PiotrZSL, if you think that this patch, and its dependency are ready
to land then please can you do so? I don't have permission to do so
myself.

mikecrowe marked an inline comment as done.Jun 25 2023, 9:23 AM
mikecrowe added inline comments.Jun 25 2023, 9:54 AM
clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
65

This is going to write the default value set in the constructor if ReplacementPrintFunction is std::print or ReplacementPrintlnFunction is std::println even if PrintHeader was not specified in the configuration. I don't know how much of a problem this is.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 25 2023, 12:12 PM

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.

PiotrZSL reopened this revision.Jun 25 2023, 12:43 PM

@thakis Thank you.

clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
10–16

Those includes need to be removed. We cannot use system includes, some dummy one can be used only.

This revision is now accepted and ready to land.Jun 25 2023, 12:43 PM
mikecrowe updated this revision to Diff 534370.Jun 25 2023, 1:31 PM

Add dummy <inttypes.h> header for tests

This header is hopefully correct for LP64 and P64, but I haven't tested it
on anything other than x86_64 Linux yet.

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 get away with including <inttypes.h> from a test since it is a compiler header rather than a libc header.

I've added a potential fix, but I don't have a Windows machine (or many other targets) to test it on. Is there a way to get this change to run through the buildbots without landing it?

mikecrowe marked an inline comment as done.Jun 25 2023, 1:48 PM
mikecrowe added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
10–16

I believe that <inttypes.h> is the only one that isn't present in test/clang-tidy/checkers/Inputs/Headers/. Hopefully adding it will have been sufficient.

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).

This revision was automatically updated to reflect the committed changes.
PiotrZSL added inline comments.Jun 26 2023, 2:12 AM
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cstddef
12

We shouldn't include this stddef.h, it's being included from system headers.

PiotrZSL reopened this revision.Jun 26 2023, 2:52 AM
This revision is now accepted and ready to land.Jun 26 2023, 2:52 AM
mikecrowe marked an inline comment as done.Jun 26 2023, 4:27 AM

It looks like the char test is failing on targets where unadorned char is unsigned. I have access to two such machines, so I ought to be able to reproduce this myself (either on the up-to-date slow one, or the fast one that lacks a sufficiently-new cmake.)

@thakis, http://45.33.8.238/win/80313/summary.html makes it looks like Windows may be happy now, but I may be misinterpreting the results.

mikecrowe added inline comments.Jun 26 2023, 9:41 AM
clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
170

This should have a cast to signed char if StrictMode=true. That cast is currently there by accident on targets where char is unsigned, but not on targets where char is signed. This is a real problem, but luckily one that's easy to fix.

Fix tests on targets where char is unsigned

We need to cast chars to to signed char if we want to guarantee that
they are formatted as signed numbers on all targets.

Improve char formatting tests a little too.

Add dummy stddef.h header

This revision was landed with ongoing or failed builds.Jun 26 2023, 12:05 PM
This revision was automatically updated to reflect the committed changes.

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 use of the return value. I think this means that I need to only match on calls that don't make use of the return value. I shall try to do that.

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 use of the return value. I think this means that I need to only match on calls that don't make use of the return value. I shall try to do that.

You can provide warning, but without fix. But that would need to be put into documentation.
If we lucky, and there will be no issues with current version, then you can put this in separate patch.
Anyway I do not think that many project use printf, most probably they use std::cout, fmt::format or some other custom wrapper around something.