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

[mikecrowe]

Summary

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

[mikecrowe]

I can squash this with https://reviews.llvm.org/D143342 if required.

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

152–154

`std::print, std::format or other functions listed in StringParameterFunction` check option.

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

14–30

too much noise in this, configuration example is not needed.
reference to C++20 or std::format/std::print also...

Strongly consider something like:
"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. Default value is empty string."

[PiotrZSL]

My only complain is documentation, changes in code and tests are correct.
Build failed mainly due to conflicts in previous change.
It should get green once prev patch will land, and this will be re-based.

I strongly suggest using arc (less issues with patches).

[mikecrowe]

Rebase.

[mikecrowe]

In D145885#4187679, @PiotrZSL wrote:

My only complain is documentation, changes in code and tests are correct.
Build failed mainly due to conflicts in previous change.
It should get green once prev patch will land, and this will be re-based.

Thanks for the comments. I will address them. I've rebased this change on top of the rebase of the previous one. Maybe that will help. Git didn't report any conflicts when I rebased.

I strongly suggest using arc (less issues with patches).

I have been using arc. I couldn't find a way to make it submit updates to more than one change at a time. I've done these two with git checkout HEAD~1 ; arc diff ; git checkout mybranch; arc diff. Is there a better way?

[mikecrowe]

Documentation updates suggested by @PiotrZSL. Rebase.

[PiotrZSL]

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

18

Put function names in double `, like fmt::format, spdlog::logger::info`

[mikecrowe]

Quote types and functions correctly in redundant-string-cstr.rst.

[PiotrZSL]

LGTM

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

 
 #include "../ClangTidyCheck.h"
 
 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
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTSTRINGCSTRCHECK_H

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

 //===- RedundantStringCStrCheck.cpp - Check for redundant c_str calls -----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements a check for redundant calls of c_str() on strings.
 //
 //===----------------------------------------------------------------------===//
 
 #include "RedundantStringCStrCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::readability {
 
 namespace {

 }
 
 AST_MATCHER(MaterializeTemporaryExpr, isBoundToLValue) {
   return Node.isBoundToLvalueReference();
 }
 
 } // 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*'.
   const auto StringDecl = type(hasUnqualifiedDesugaredType(recordType(
       hasDeclaration(cxxRecordDecl(hasName("::std::basic_string"))))));
   const auto StringExpr =
       expr(anyOf(hasType(StringDecl), hasType(qualType(pointsTo(StringDecl)))));
 
Finder->addMatcher(
               // The only argument must have the form x.c_str() or p->c_str()
               // where the method is string::c_str().  StringRef also has
               // a constructor from string which is more efficient (avoids
               // strlen), so we can construct StringRef from the string
               // directly.
               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);
   }
 }
 
 void RedundantStringCStrCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`cert-msc33-c
   <clang-tidy/checks/cert/msc33-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 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-tidy/checks/bugprone/dynamic-static-initializers>` check.
   Global options of the same name should be used instead.
 
 - Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions`
   in :doc:`bugprone-suspicious-include
   <clang-tidy/checks/bugprone/suspicious-include>` check.

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

 .. title:: clang-tidy - readability-redundant-string-cstr
 
 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

+// 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());
+}