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

[mikecrowe]

Summary

std::format (C++20) and std::print (C++23) are perfectly happy to accept
std::string arguments. Converting them to C-style strings by calling
c_str() is unnecessary and may cause extra walking of the string to
determine its length.

Depends on D144216

[mikecrowe]

This check can also work trivially for fmt::format and fmt::print[1] too (in fact, that's what I'm actually using), if the project would allow them to be added too.

[1] https://fmt.dev/

[carlosgalvezp]

Please document change in Release Notes, as well as in the check documentation, together with its limitations (can only handle 1 argument at a time).

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

186

Nit: add a comment explaining what this matcher does, for consistency with how it's done with the other matchers above.

[mikecrowe]

carlosgalvezp wrote:

Please document change in Release Notes, as well as in the check documentation, together with its limitations (can only handle 1 argument at a time).

Hopefully I've done those things.

[carlosgalvezp]

LGTM, thanks for the fix! Might be good to wait a few days for other reviewers to have a look or give suggestions on how to improve the issue of having to do one parameter at a time.

[njames93]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

190–191

192

The limitation about only transforming the first argument can be alleviated by using the forEachArgumentWithParam matcher

clang-tools-extra/docs/ReleaseNotes.rst

152–154

Please address this needless restriction, see above comment

clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp

336 ↗

(On Diff #494937)

Again this test case would need updating to handle applying all fixes

[mikecrowe]

The limitation about only transforming the first argument can be alleviated by using the forEachArgumentWithParam matcher

I shall try to make that work. My initial attempts were not successful, but there's still more to try. If I can't make it work I'll ask for more help here.

Thanks.

[mikecrowe]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

192

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.

[njames93]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

192

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 here

[mikecrowe]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

192

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.

[mikecrowe]

Incorporate feedback from Nathan James:

• Simplify callExpr pattern in matcher.
• Use forEachArgumentWithParam so that check works on all arguments and remove mentions of previous limitation in commit message and documentation.
• Make lit test use closer approximation to standard first argument for std::format and std::print rather than const char *.
• Add wide-character string tests.
• Other minor improvements to tests.

[njames93]

Just a few more points then it should be good to land

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

185–194

Can this be wrapped in a check to make sure it only runs in c++20

Likewise ::std::print is only c++23, so maybe:

getLangOpts().CPlusPlus2B ? hasAnyName("::std::print", "::std::format") : hasAnyName("::std::format")

clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp

1 ↗

(On Diff #496758)

We shouldn't be removing tests from older language standards
Can all the changes made to the file be moved into a new file redundant-string-cstr-cpp20.cpp
Would likely either need to create a second file for c++23 or make use of the check-suffix to run the tests for std::print

[mikecrowe]

Thanks for the review and further suggestions.

Mike.

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

185–194

I can do that. I had (presumably incorrectly) assumed that since and use of std::print and std::format in any previous version would be UB then it was safe to apply the check regardless of the C++ version.

clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp

1 ↗

(On Diff #496758)

My brief reading up on check-suffix implies that it could be used to keep everything in redundant-string-cstr.cpp, but that file is getting rather large anyway. Having redundant-string-cstr-cpp20.cpp also containing C++23 tests might be a bit confusing. Having redundant-string-cstr-cpp23.cpp too would mean moving the format_args stuff to a header (which might be beneficial regardless.)

Anyway, I shall have a play and see what I can make work.

Do I need to add tests to ensure that the checks don't trigger when running against earlier standard versions?

[mikecrowe]

I ended up splitting out the std::format and std::print tests to their own file which meant that I didn't need to modify the existing redundant-string-cstr.cpp file in this commit. (Though of course I had to extract the <string> header in D144216 first.) Let me know if you don't like the split.

I also slightly re-ordered the tests and removed a few incorrect comments that incorrectly survived the switch to using forEachArgumentWithParam.

[PiotrZSL]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

193

Please introduce configuration option to specify custom functions.
For example if some project (like mine) is wrapping fmt::format with some variadic template function, then such function could be specified.
Same goes to things like some loggers.

Check utils/OptionsUtils.h for configuration, and utils/Matchers.h (matchesAnyListedName)

[mikecrowe]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

193

Please introduce configuration option to specify custom functions.
For example if some project (like mine) is wrapping fmt::format with some variadic template function, then such function could be specified.

That's exactly where this change originated (as part of my clang-tidy fmt fork, which I hope to submit for review soon once I've made it configurable too and improved the test cases.)

Same goes to things like some loggers.

Check utils/OptionsUtils.h for configuration, and utils/Matchers.h (matchesAnyListedName)

Thanks for the pointer. I shall study those files for how to support this.

Do you think that the change can land like in its current state first? Or would you prefer that the configuration option is added at the same time?

[PiotrZSL]

"Do you think that the change can land like in its current state first? Or would you prefer that the configuration option is added at the same time?"

Code is fine, probably If would would write this, then I would bother to split into C++20 and C++2B simply because std::print wouldn't compile if it wouldn't be available. So I would just use hasAnyName.
But code looks fine, configuration could be added later...

[mikecrowe]

In D143342#4186809, @PiotrZSL wrote:

Code is fine, probably If would would write this, then I would bother to split into C++20 and C++2B simply because std::print wouldn't compile if it wouldn't be available. So I would just use hasAnyName.
But code looks fine, configuration could be added later...

Thanks. I originally wrote it like that, but @njames93 requested that I checked the version, so I did. :) It doesn't do any harm and might avoid confusion.

[mikecrowe]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

193

Please introduce configuration option to specify custom functions.
For example if some project (like mine) is wrapping fmt::format with some variadic template function, then such function could be specified.

I could add some sort of readability-redundant-string-cstr.PrintFunction option or readability-redundant-string-cstr.FormatFunction option, but reducing this to its fundamental behaviour would be "a function that takes const char * arguments that is also willing to take std::string arguments". I'm struggling to find a sensible name for such an option. Maybe readability-redundant-string-cstr.StdStringAcceptingFunction=::fmt::format? Or just readability-redundant-string-cstr.FunctionCall=::fmt::format?

Same goes to things like some loggers.

Loggers may be classes, so there would need to be an option that specifies a class name (or even a base class name) and a method name (which may be an operator.) See this hard-coded example (If it's not obvious, there's a description in the README. In such cases the options could be readability-redundant-string-cstr.Class=::BaseTrace and readability-redundant-string-cstr.Method=Log or readability-redundant-string-cstr.Operator=(), but then it would be hard to tie together the right classes and methods. That could be avoided with something like readability-redundant-string-cstr.MemberFunctionCall=::BaseTrace::operator(),::NullTrace::operator() and some parsing I suppose.

Regardless, I'll try and get the simple case working and await suggestions for appropriate option names.

Thanks again for the suggestions.

[PiotrZSL]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

193

I asked AI, and it suggested readability-redundant-string-cstr.FormattingFunctionsList.
Also, I thing that matchesAnyListedName should support things like '::NullTrace::operator()', so one option should be sufficient.

FormattingFunctionsList:
A semicolon-separated list of (fully qualified) function/method/operator names, with the requirement that
any parameter capable of accepting a 'const char*' input should also be able to accept 'std::string' or
'std::string_view' inputs, or proper overload candidates that can do so should exist.

[mikecrowe]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

193

Far be it from me to question the wisdom of the AI :) , but my point was that the check works for any function that accepts const char * parameters that could equally well be std::string parameters. It's not limitted to formatting functions. Despite this, I suspect that if the majority use case is formatting functions then maybe it's more helpful to use a name connected with that for discoverability.

In my prototype (which is proving easier to implement than I expected so far), I've used readability-redundant-string-cstr.StringParameterFunctions, which I can't say I'm entirely happy with either.

[PiotrZSL]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

193

StringParameterFunctions can be too....

[mikecrowe]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

193

My first go at an implementation is in https://reviews.llvm.org/D145885 (but could be squashed into this change if desired, since it hasn't landed yet.)

[PiotrZSL]

Correct commit message and this could land.

[mikecrowe]

In D143342#4187641, @PiotrZSL wrote:

Correct commit message and this could land.

I've read through all the comments and the commit message itself and I can't spot what's wrong with the commit message, except perhaps for its verbosity. What would you like me to correct?

[PiotrZSL]

"The dummy implementations of std::format and std::print require C++20,
so it's easier to test them in a separate
redundant-string-cstr-format.cpp check file. This also requires a small
improvement to the basic_string_view implementation in the <string>
dummy header.

With these changes, I was able to successfully run the
readability-redundant-string-cstr check on a real source file that
compiled successfully with what is destined to become GCC 13, and saw
the expected:"

You shouldn't explain your self why you choose this or other implementation, or pasting output from console.
Commit message should contain only description about changes.
All those explanation you can always put into review as comment.

https://llvm.org/docs/DeveloperPolicy.html#commit-messages:
"The body should be concise, but explanatory, including a complete reasoning. Unless it is required to understand the change, examples, code snippets and gory details should be left to bug comments, web review or the mailing list."

[mikecrowe]

Thank you for your patience.

[PiotrZSL]

You will need to rebase this, there are conflicts...

[mikecrowe]

Rebase. No conflicts.