[clang-tidy] Support readability-redundant-string-cstr.StringParameterFunctions option

Add StringParameterFunctions option to allow the
readability-redundant-string-cstr check to work with library functions
such as fmt::format and spdlog::logger:info that are able to support
std::string arguments in addition to const char * ones.

Depends on D143342

Reviewed By: PiotrZSL

Differential Revision: https://reviews.llvm.org/D145885

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

@@ -11,6 +11,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "RedundantStringCStrCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/FixIt.h"
 
@@ -69,6 +71,17 @@ AST_MATCHER(MaterializeTemporaryExpr, isBoundToLValue) {
 
 } // end namespace
 
+RedundantStringCStrCheck::RedundantStringCStrCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      StringParameterFunctions(utils::options::parseStringList(
+          Options.get("StringParameterFunctions", ""))) {
+  if (getLangOpts().CPlusPlus20)
+    StringParameterFunctions.push_back("::std::format");
+  if (getLangOpts().CPlusPlus2b)
+    StringParameterFunctions.push_back("::std::print");
+}
+
 void RedundantStringCStrCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   // Match expressions of type 'string' or 'string*'.
@@ -183,15 +196,13 @@ void RedundantStringCStrCheck::registerMatchers(
               hasArgument(0, StringCStrCallExpr))),
       this);
 
-  if (getLangOpts().CPlusPlus20) {
+  if (!StringParameterFunctions.empty()) {
     // Detect redundant 'c_str()' calls in parameters passed to std::format in
     // C++20 onwards and std::print in C++23 onwards.
     Finder->addMatcher(
         traverse(TK_AsIs,
-                 callExpr(callee(functionDecl(
-                              getLangOpts().CPlusPlus2b
-                                  ? hasAnyName("::std::print", "::std::format")
-                                  : hasName("::std::format"))),
+                 callExpr(callee(functionDecl(matchers::matchesAnyListedName(
+                              StringParameterFunctions))),
                           forEachArgumentWithParam(StringCStrCallExpr,
                                                    parmVarDecl()))),
         this);

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

@@ -16,13 +16,15 @@ namespace clang::tidy::readability {
 /// Finds unnecessary calls to `std::string::c_str()`.
 class RedundantStringCStrCheck : public ClangTidyCheck {
 public:
-  RedundantStringCStrCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  RedundantStringCStrCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::vector<StringRef> StringParameterFunctions;
 };
 
 } // namespace clang::tidy::readability

clang-tools-extra/docs/ReleaseNotes.rst

@@ -149,7 +149,8 @@ Changes in existing checks
 - Improved :doc:`readability-redundant-string-cstr
   <clang-tidy/checks/readability/redundant-string-cstr>` check to recognise
   unnecessary ``std::string::c_str()`` and ``std::string::data()`` calls in
-  arguments to ``std::print`` and ``std::format``.
+  arguments to ``std::print``, ``std::format`` or other functions listed in
+  the ``StringParameterFunction`` check option.
 
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`bugprone-dynamic-static-initializers

clang-tools-extra/docs/clang-tidy/checks/readability/redundant-string-cstr.rst

@@ -5,3 +5,16 @@ readability-redundant-string-cstr
 
 
 Finds unnecessary calls to ``std::string::c_str()`` and ``std::string::data()``.
+
+Options
+-------
+
+.. option:: StringParameterFunctions
+
+   A semicolon-separated list of (fully qualified) function/method/operator
+   names, with the requirement that any parameter currently accepting a
+   ``const char*`` input should also be able to accept ``std::string``
+   inputs, or proper overload candidates that can do so should exist. This
+   can be used to configure functions such as ``fmt::format``,
+   ``spdlog::logger::info``, or wrappers around these and similar
+   functions. The default value is the empty string.

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

@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:             [{key: readability-redundant-string-cstr.StringParameterFunctions, \
+// RUN:               value: '::fmt::format; ::fmt::print; ::BaseLogger::operator(); ::BaseLogger::Log'}] \
+// RUN:             }" \
+// RUN:   -- -isystem %clang_tidy_headers
+#include <string>
+
+namespace fmt {
+  inline namespace v8 {
+    template<typename ...Args>
+    void print(const char *, Args &&...);
+    template<typename ...Args>
+    std::string format(const char *, Args &&...);
+  }
+}
+
+namespace notfmt {
+  inline namespace v8 {
+    template<typename ...Args>
+    void print(const char *, Args &&...);
+    template<typename ...Args>
+    std::string format(const char *, Args &&...);
+  }
+}
+
+void fmt_print(const std::string &s1, const std::string &s2, const std::string &s3) {
+  fmt::print("One:{}\n", s1.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}fmt::print("One:{}\n", s1);
+
+  fmt::print("One:{} Two:{} Three:{}\n", s1.c_str(), s2, s3.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:58: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}fmt::print("One:{} Two:{} Three:{}\n", s1, s2, s3);
+}
+
+// There's no c_str() call here, so it shouldn't be touched
+void fmt_print_no_cstr(const std::string &s1, const std::string &s2) {
+    fmt::print("One: {}, Two: {}\n", s1, s2);
+}
+
+// This isn't fmt::print, so it shouldn't be fixed.
+void not_fmt_print(const std::string &s1) {
+    notfmt::print("One: {}\n", s1.c_str());
+}
+
+void fmt_format(const std::string &s1, const std::string &s2, const std::string &s3) {
+  auto r1 = fmt::format("One:{}\n", s1.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}auto r1 = fmt::format("One:{}\n", s1);
+
+  auto r2 = fmt::format("One:{} Two:{} Three:{}\n", s1.c_str(), s2, s3.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:53: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:69: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}auto r2 = fmt::format("One:{} Two:{} Three:{}\n", s1, s2, s3);
+}
+
+// There's are c_str() calls here, so it shouldn't be touched
+void fmt_format_no_cstr(const std::string &s1, const std::string &s2) {
+    fmt::format("One: {}, Two: {}\n", s1, s2);
+}
+
+// This is not fmt::format, so it shouldn't be fixed
+std::string not_fmt_format(const std::string &s1) {
+    return notfmt::format("One: {}\n", s1.c_str());
+}
+
+class BaseLogger {
+public:
+  template <typename... Args>
+  void operator()(const char *fmt, Args &&...args) {
+  }
+
+  template <typename... Args>
+  void Log(const char *fmt, Args &&...args) {
+  }
+};
+
+class DerivedLogger : public BaseLogger {};
+class DoubleDerivedLogger : public DerivedLogger {};
+typedef DerivedLogger TypedefDerivedLogger;
+
+void logger1(const std::string &s1, const std::string &s2, const std::string &s3) {
+  BaseLogger LOGGER;
+
+  LOGGER("%s\n", s1.c_str(), s2, s3.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:34: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}LOGGER("%s\n", s1, s2, s3);
+
+  DerivedLogger LOGGER2;
+  LOGGER2("%d %s\n", 42, s1.c_str(), s2.c_str(), s3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:38: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}LOGGER2("%d %s\n", 42, s1, s2, s3);
+
+  DoubleDerivedLogger LOGGERD;
+  LOGGERD("%d %s\n", 42, s1.c_str(), s2, s3.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:42: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}LOGGERD("%d %s\n", 42, s1, s2, s3);
+
+  TypedefDerivedLogger LOGGERT;
+  LOGGERT("%d %s\n", 42, s1.c_str(), s2, s3.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:42: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}LOGGERT("%d %s\n", 42, s1, s2, s3);
+}
+
+void logger2(const std::string &s1, const std::string &s2) {
+  BaseLogger LOGGER3;
+
+  LOGGER3.Log("%s\n", s1.c_str(), s2.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:35: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}LOGGER3.Log("%s\n", s1, s2);
+
+  DerivedLogger LOGGER4;
+  LOGGER4.Log("%d %s\n", 42, s1.c_str(), s2.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-MESSAGES: :[[@LINE-2]]:42: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}LOGGER4.Log("%d %s\n", 42, s1, s2);
+}
+
+class NotLogger {
+public:
+  template <typename... Args>
+  void operator()(const char *fmt, Args &&...args) {
+  }
+
+  template <typename... Args>
+  void Log(const char *fmt, Args &&...args) {
+  }
+};
+
+void Log(const char *fmt, ...);
+
+void logger3(const std::string &s1)
+{
+  // Not BaseLogger or something derived from it
+  NotLogger LOGGER;
+  LOGGER("%s\n", s1.c_str());
+  LOGGER.Log("%s\n", s1.c_str());
+
+  // Free function not in StringParameterFunctions list
+  Log("%s\n", s1.c_str());
+}