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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:189
+                              getLangOpts().CPlusPlus2b
+                                  ? hasAnyName("::std::print", "::std::format")
+                                  : hasName("::std::format"))),
----------------
mikecrowe wrote:
> mikecrowe wrote:
> > PiotrZSL wrote:
> > > 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)
> > > 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 [[ https://github.com/mikecrowe/clang-tidy-fmt | 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?
> > 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 [[ https://github.com/mikecrowe/clang-tidy-fmt/blob/7ace8a3ff41e9679104fe558835b0ef3cb33d969/clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp#L196 | this hard-coded example ]] (If it's not obvious, there's a description in the [[ https://github.com/mikecrowe/clang-tidy-fmt/blob/7ace8a3ff41e9679104fe558835b0ef3cb33d969/README.md?plain=1#L103 | 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.
> 
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.


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

https://reviews.llvm.org/D143342