mikecrowe 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()),
----------------
PiotrZSL wrote:
> 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.
> `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 [[
https://reviews.llvm.org/D154884 | D154884 ]] .
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154287/new/
https://reviews.llvm.org/D154287
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
