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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits