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

[Mike Crowe via Phabricator via cfe-commits]

mikecrowe created this revision.
mikecrowe added a reviewer: njames93.
mikecrowe added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
mikecrowe requested review of this revision.
Herald added a subscriber: cfe-commits.

std::format (C++20) and std::print (C++23) are perfectly happy to accept
std::string parameters. Converting them to C-style strings by calling
c_str() is unnecessary and may cause extra walking of the string to
determine its length.

It's straightforward to teach readability-redundant-string-cstr to
identify the first such unnecessary call to c_str() in the parameter
list, but I was unable to come up with a way that worked for all the
parameters.

I was unable to come up with a way to make libstdc++'s format_string
first parameter to std::format and std::print work in the
redundant-string-cstr.cpp lit check. Using const char * is sufficient
though since that isn't the argument we're interested in.

I was able to successfully run the readability-redundant-string-cstr
check on a real source file that compiled successfully with what is
destined to become GCC 13, and saw the expected:

--8<--
/home/mac/git/llvm-project/build/bin/clang-tidy -checks=-*,readability-redundant-string-cstr format.cpp -- --gcc-toolchain=/home/mac/gcc-git
6 warnings generated.
/home/mac/src/random-hacks/std-format/format.cpp:13:39: warning: redundant call to 'c_str' [readability-redundant-string-cstr]

  std::puts(std::format("Hello {}", get().c_str()).c_str());
                                    ^~~~~~~~~~~~~
                                    get()

Suppressed 5 warnings (5 with check filters).
-->8--


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143342

Files:
  clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
@@ -291,3 +291,59 @@
   Foo.func2((Str.c_str()));
 }
 } // namespace PR45286
+
+namespace std {
+  template<typename ...Args>
+  void print(const char *, Args &&...);
+  template<typename ...Args>
+  std::string format(const char *, Args &&...);
+}
+
+namespace notstd {
+  template<typename ...Args>
+  void print(const char *, Args &&...);
+  template<typename ...Args>
+  std::string format(const char *, Args &&...);
+}
+
+void std_print(const std::string &s1, const std::string &s2, const std::string &s3) {
+  std::print("One:{}\n", s1.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}std::print("One:{}\n", s1);
+
+  // Ideally we'd fix both the second and fourth parameters here, but that doesn't work.
+  std::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-FIXES: {{^  }}std::print("One:{} Two:{} Three:{}\n", s1, s2, s3.c_str());
+}
+
+// There's no c_str() call here, so it shouldn't be touched.
+void std_print_no_cstr(const std::string &s1, const std::string &s2) {
+  std::print("One: {}, Two: {}\n", s1, s2);
+}
+
+// This isn't std::print, so it shouldn't be fixed.
+void not_std_print(const std::string &s1) {
+  notstd::print("One: {}\n", s1.c_str());
+}
+
+void std_format(const std::string &s1, const std::string &s2, const std::string &s3) {
+  auto r1 = std::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 = std::format("One:{}\n", s1);
+
+  // Ideally we'd fix both the second and fourth parameters here, but that doesn't work.
+  auto r2 = std::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-FIXES: {{^  }}auto r2 = std::format("One:{} Two:{} Three:{}\n", s1, s2, s3.c_str());
+}
+
+// There's are c_str() calls here, so it shouldn't be touched.
+std::string std_format_no_cstr(const std::string &s1, const std::string &s2) {
+  return std::format("One: {}, Two: {}\n", s1, s2);
+}
+
+// This is not std::format, so it shouldn't be fixed.
+std::string not_std_format(const std::string &s1) {
+  return notstd::format("One: {}\n", s1.c_str());
+}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -178,6 +178,14 @@
               // directly.
               hasArgument(0, StringCStrCallExpr))),
       this);
+
+  Finder->addMatcher(
+      traverse(TK_AsIs,
+               callExpr(anyOf(callee(functionDecl(hasName("::std::print"))),
+                              callee(functionDecl(hasName("::std::format")))),
+                        hasAnyArgument(materializeTemporaryExpr(
+                            has(StringCStrCallExpr))))),
+      this);
 }
 
 void RedundantStringCStrCheck::check(const MatchFinder::MatchResult &Result) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D143342.494910.patch
Type: text/x-patch
Size: 3639 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230205/7eba5726/attachment.bin>