[PATCH] D143342: [clang-tidy] Support std::format and std::print in readability-redundant-string-cstr

[Mike Crowe via Phabricator via cfe-commits]

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