[clang-tidy] Add modernize-use-std-format check

[mikecrowe]

Summary

Add a new clang-tidy check that converts absl::StrFormat (and similar
functions) to std::format (and similar functions.)

Split the configuration of FormatStringConverter out to a separate
Configuration class so that we don't risk confusion by passing two
boolean configuration parameters into the constructor. Add
AllowTrailingNewlineRemoval option since we never want to remove
trailing newlines in this check.

[PiotrZSL]

To be honest, I do not see to much use case for this check. But that's fine.

clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp

47–52

NOTE: put those things directly into addMatcher

51

put hasArgument before callee (performance), you can also add unless(argumentCountIs(0)).

70

keep binding consistent StrFormat vs func_decl

79

Use callExpr location, not calee for diagnostic if possible (should point to same place anyway).

82

using `<< OldFunction` should be sufficient and more safe, same in line 88

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

632–633

what if it's ends with Windows new line style ? Will it work ?

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

28

is this link correct ? shouldn't this be put into doc:, verify if its working

34

If not implemented, do not add it

[Eugene.Zelenko]

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

7

Please synchronize first statement with Release Notes.

59

Please highlight false with single back-ticks.

71

Single back-ticks for option value.

85

Ditto.

[mikecrowe]

Thanks for the reviews!

clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp

82

I don't think that's the same. If I use just OldFunction then I get the template arguments in strings like StrFormat<const char *, const char *, const char *, const char *> and sprintf<char[6], int> when I just want StrFormat and sprintf respectively.

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

632–633

Good point. Your question actually relates to the modernize-use-std-print check only since config.AllowTrailingNewlineRemoval is always false in this check.

std::println is described as just appending \n to the end of the formatted string when written to the desired output. The code here means that printf("A\r\n") would be converted to std::println("A\r") which would behave in the same way, though I would admit that it's rather confusing.

This situation isn't really Windows-specific. Most the of the time a POSIX tty would be in cooked mode, so the \r would be added automatically before the \n. However, when the tty is in raw mode using \r\n line endings would be necessary there too, just as they might be on Windows when writing to a binary stream.

I think that the least confusing outcome would be for this code to not consider format strings that end with \r\n to be suitable for conversion to std::println and let them use std::print instead. I think that this better reflects the intent of the existing code.

I will prepare a separate change for this.

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

28

It was wrong, but I believe that I've fixed it.

34

It is implemented, I just forgot to remove the TODO.

[mikecrowe]

Address review comments and improve documentation.

[mikecrowe]

clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp

47

This matcher also matches the operator+ call in:

std::string A(const std::string &in)                                                                                                                                            
{                                                                                                                                                                               
    return "_" + in;                                                                                                                                                            
}

which causes an assertion failure:

clang-tidy: /home/mac/git/llvm-project/clang/include/clang/AST/Decl.h:275: llvm::StringRef clang::NamedDecl::getName() const: Assertion `Name.isIdentifier() && "Name is not a simple identifier"' failed.

when the StrFormatLikeFunctions option is set to an unqualified name:

-config="{CheckOptions: [{key: modernize-use-std-format.StrictMode, value: false}, {key: modernize-use-std-format.StrFormatLikeFunctions, value: 'strprintf'}]}"

MatchesAnyListedNameMatcher::NameMatcher::match calls NamedDecl.getName() which presumably raises the assertion due to the operator+ not having a name (that's mentioned in the source anyway.)

I'm unsure whether I should be narrowing the matcher here so that it guaranteed to not try calling matchesAnyListedName on something that lacks a name, or whether MatchesAnyListedNameMatcher ought to be more tolerant of being called in such situations.

I note that HasNameMatcher has rather more elaborate code for generating the name than MatchesAnyListedNameMatcher does.

This problem also affects modernize-use-std-print, but due to the need for there to be no return value in that check it requires somewhat-unlikely code like:

void A(const std::string &in)
{
  "_" + in;
}

Do you have any advice? Given that this problem affects a check that has already landed should I open a bug?

48

I need to add an isOrdinary() call here like modernize-use-std-print has.

[PiotrZSL]

clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp

47

unless(hasName("")) could do a trick, or create own matcher to verify first if function got name.
Probably similar issues can be with cxxConversionDecl.

Other best option would be to change MatchesAnyListedNameMatcher::NameMatcher::match to verify if NamedDecl got name before calling it.

[mikecrowe]

clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp

47

unless(hasName("")) could do a trick, or create own matcher to verify first if function got name.

That fails with a similar assertion failure.

Probably similar issues can be with cxxConversionDecl.

I didn't really understand that one.

Other best option would be to change MatchesAnyListedNameMatcher::NameMatcher::match to verify if NamedDecl got name before calling it.

That's easy and I think it's the best solution since it saves every check having to defend against this. I've done that in D154884 .

[mikecrowe]

Use isOrdinary in stringLiteral() match on first argument

[mikecrowe]

Rebase on top of D156616 and add more tests

[mikecrowe]

Rebase and fix minor doc conflicts.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

177

Please keep alphabetical order (by check name) in this section.

[mikecrowe]

Fix ReleaseNotes order and remove unnecessary .html from list.rst