mikecrowe added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:189
+ getLangOpts().CPlusPlus2b
+ ? hasAnyName("::std::print", "::std::format")
+ : hasName("::std::format"))),
----------------
PiotrZSL wrote:
> mikecrowe wrote:
> > PiotrZSL wrote:
> > > 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.
> > 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.
> StringParameterFunctions can be too....
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.)
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
