[PATCH] D154287: [clang-tidy] Add modernize-use-std-format check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp:46
+void UseStdFormatCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      callExpr(argumentCountAtLeast(1), hasArgument(0, stringLiteral()),
----------------
mikecrowe wrote:
> 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?
`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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154287