mikecrowe added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:188
+ callee(functionDecl(hasName("::std::format")))),
+ hasAnyArgument(materializeTemporaryExpr(
+ has(StringCStrCallExpr))))),
----------------
njames93 wrote:
> mikecrowe wrote:
> > njames93 wrote:
> > > The limitation about only transforming the first argument can be
> > > alleviated by using the `forEachArgumentWithParam` matcher
> > Thanks for the hint. I did try to overcome this limitation but failed to
> > find `forEachArgumentWithParam`.
> >
> > I think that I've managed to make `forEachArgumentWithParam` work, but only
> > if I lose the `materializeTemporaryExpr`. I hope that's not essential. I've
> > ended up with:
> >
> > ```C++
> > Finder->addMatcher(
> >
> >
> > traverse(TK_AsIs,
> >
> >
> > callExpr(callee(functionDecl(hasAnyName("::std::print",
> > "::std::format"))),
> >
> > forEachArgumentWithParam(StringCStrCallExpr,
> > parmVarDecl()))),
> >
> > this);
> >
> >
> > ```
> > I suspect that there's something better than the `parmVarDecl()` second
> > parameter to `forEachArgumentWithParam` though.
> `parmVarDecl` is exactly what you need here.
>
> My understanding of the temporary expressions isn't perfect, but from what I
> can gather, we shouldn't need temporaries when calling the `.c_str()`method
> for all but the first argument.
>
> This does highlight an issue with your test code though.
> `std::format` and `std::print` take a `std::format_string` as the first
> argument, but in your tests you have it as `const char *`
> Anyway I think this means the first argument in real world code would result
> in a `materializeTemporaryExpr`
> See [[ https://en.cppreference.com/w/cpp/utility/format/basic_format_string |
> here ]]
I mentioned that I'd failed to use a genuine type for the first parameter in my
commit message. However, I've now tried again, and I think that I've got
something working that is much closer to that required by the standard.
I tried to come up with something that I could call `c_str()` on to pass as the
first argument, but couldn't come up with anything that g++ thought was a
constant expression. I tried:
```
consteval std::string f()
{
return {"Hi {}"};
}
std::puts(std::format(f().c_str(), get().c_str());
```
but g++ still claims:
```
format.cpp:22:28: error:
‘std::string{std::__cxx11::basic_string<char>::_Alloc_hider{((char*)(&<anonymous>.std::__cxx11::basic_string<char>::<anonymous>.std::__cxx11::basic_string<char>::<unnamed
union>::_M_local_buf))}, 5, std::__cxx11::basic_string<char>::<unnamed
union>{char [16]{'H', 'i', ' ', '{', '}', 0, '\000', '\000', '\000', '\000',
'\000', '\000', '\000', '\000', '\000', '\000'}}}’ is not a constant expression
22 | std::puts(std::format(f().c_str(), get().c_str());
| ~^~
```
So I think that the risk of inadvertently removing `c_str()` from the first
argument is low, given that such a call appears not to be legal anyway. Even if
there does turn out to be a way to do so, I think that removing the `c_str()`
call probably ought to be expected to work too.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143342/new/
https://reviews.llvm.org/D143342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
