[clang-tidy] Add support for different char-types for the readability-redundant-string-cstr checker.

[etienneb]

Summary

The current checker is able to recognize std::string but does not recognize other string variants.
This patch is adding the support for any string defined with basic_string without considering the
the underlying char type.

The most common variant is: 'std::wstring' based on 'wchar_t'.

There are also other string variants added to the standard: u16string, u32string, etc...

[dblaikie]

can you just match on the name of the template instead?

[mamai]

nit: in summary, consiring -> considering ?

test/clang-tidy/readability-redundant-string-cstr.cpp

54

Isn't this a copy-paste error to reference f1 ? Same question for line 62 and 70...

[etienneb]

Fix unittests.

[etienneb]

In D18412#381843, @dblaikie wrote:

can you just match on the name of the template instead?

I'm not sure to get your point?

Are you proposing to match "basic_string" instead of the whole regexp?
I'm in favor of that change as I don't see any counter example or any false positive.

In fact, many checkers are using hasName("basic_string").
I just kept the change as minimal as possible to keep the same semantic and allow matching more cases.

Example from RedundantStringInitCheck.cpp:

// Match string constructor.
const auto StringConstructorExpr = expr(anyOf(
    cxxConstructExpr(argumentCountIs(1),
                     hasDeclaration(cxxMethodDecl(hasName("basic_string")))),

test/clang-tidy/readability-redundant-string-cstr.cpp

54

Good catch.

[alexfh]

clang-tidy/readability/RedundantStringCStrCheck.cpp

91

hasName("basic_string") should be enough. Same for c_str.

[etienneb]

Change name/decl matchers to 'basic_string'.

[etienneb]

remove dead-code

[alexfh]

clang-tidy/readability/RedundantStringCStrCheck.cpp

103

Why is it still matchesName(StringCStrMethod)?

[etienneb]

fix hasName.

[etienneb]

Tests are fine. PTAnL?

clang-tidy/readability/RedundantStringCStrCheck.cpp

103

bad quick fix.

[alexfh]

LG now. Thanks!

clang-tidy/readability/RedundantStringCStrCheck.cpp

if (Text.empty())
     return std::string();
   // Add leading '*'.
   if (needParensAfterUnaryOperator(ExprNode)) {
     return (llvm::Twine("*(") + Text + ")").str();
   }
   return (llvm::Twine("*") + Text).str();
 }
 
-const char StringConstructor[] =
-    "::std::basic_string<char, std::char_traits<char>, std::allocator<char> >"
-    "::basic_string";
-
-const char StringCStrMethod[] =
-    "::std::basic_string<char, std::char_traits<char>, std::allocator<char> >"
-    "::c_str";
-
 } // end namespace
 
 namespace tidy {
 namespace readability {
 
 void RedundantStringCStrCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus)
     return;
 
+  // Match expressions of type 'string' or 'string*'.
+  const auto StringDecl =
+      cxxRecordDecl(hasName("::std::basic_string"));
+  const auto StringExpr =
+      expr(anyOf(hasType(StringDecl),
+                 hasType(qualType(pointsTo(StringDecl)))));
+
   // Match string constructor.
   const auto StringConstructorExpr = expr(anyOf(
       cxxConstructExpr(
           argumentCountIs(1),
-          hasDeclaration(cxxMethodDecl(hasName(StringConstructor)))),
+          hasDeclaration(cxxMethodDecl(hasName("basic_string")))),
       cxxConstructExpr(
           argumentCountIs(2),
-          hasDeclaration(cxxMethodDecl(hasName(StringConstructor))),
+          hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
           // If present, the second argument is the alloc object which must not
           // be present explicitly.
           hasArgument(1, cxxDefaultArgExpr()))));
 
   // Match a call to the string 'c_str()' method.
-  const auto StringCStrCallExpr = cxxMemberCallExpr(
-                            callee(memberExpr().bind("member")),
-                            callee(cxxMethodDecl(hasName(StringCStrMethod))),
-                            on(expr().bind("arg"))).bind("call");
+  const auto StringCStrCallExpr =
+      cxxMemberCallExpr(on(StringExpr.bind("arg")),
+                        callee(cxxMethodDecl(hasName("c_str"))))
+          .bind("call");
 
   Finder->addMatcher(
       cxxConstructExpr(StringConstructorExpr,
                        hasArgument(0, StringCStrCallExpr)),
       this);
 
   Finder->addMatcher(
       cxxConstructExpr(

test/clang-tidy/readability-redundant-string-cstr.cpp

 // RUN: %check_clang_tidy %s readability-redundant-string-cstr %t
 
 namespace std {
 template <typename T>
 class allocator {};
 template <typename T>
 class char_traits {};
 template <typename C, typename T, typename A>
 struct basic_string {
   basic_string();
   basic_string(const C *p, const A &a = A());
   const C *c_str() const;
 };
 typedef basic_string<char, std::char_traits<char>, std::allocator<char>> string;
+typedef basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t>> wstring;
+typedef basic_string<char16_t, std::char_traits<char16_t>, std::allocator<char16_t>> u16string;
+typedef basic_string<char32_t, std::char_traits<char32_t>, std::allocator<char32_t>> u32string;
 }
+
 namespace llvm {
 struct StringRef {
   StringRef(const char *p);
   StringRef(const std::string &);
 };
 }
 
+// Tests for std::string.
+
 void f1(const std::string &s) {
   f1(s.c_str());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to `c_str()` [readability-redundant-string-cstr]
   // CHECK-FIXES: {{^  }}f1(s);{{$}}
 }
 void f2(const llvm::StringRef r) {
   std::string s;
   f2(s.c_str());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call {{.*}}
   // CHECK-FIXES: {{^  }}std::string s;{{$}}
   // CHECK-FIXES-NEXT: {{^  }}f2(s);{{$}}
 }
 void f3(const llvm::StringRef &r) {
   std::string s;
   f3(s.c_str());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call {{.*}}
   // CHECK-FIXES: {{^  }}std::string s;{{$}}
   // CHECK-FIXES-NEXT: {{^  }}f3(s);{{$}}
 }
+void f4(const std::string &s) {
+  const std::string* ptr = &s;
+  f1(ptr->c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to `c_str()` [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}f1(*ptr);{{$}}
+}
+
+// Tests for std::wstring.
+
+void g1(const std::wstring &s) {
+  g1(s.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to `c_str()` [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}g1(s);{{$}}
+}
+
+// Tests for std::u16string.
+
+void h1(const std::u16string &s) {
+  h1(s.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to `c_str()` [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}h1(s);{{$}}
+}
+
+// Tests for std::u32string.
+
+void k1(const std::u32string &s) {
+  k1(s.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to `c_str()` [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}k1(s);{{$}}
+}
+
+// Tests on similar classes that aren't good candidates for this checker.
+
+struct NotAString {
+  NotAString();
+  NotAString(const NotAString&);
+  const char *c_str() const;
+};
+
+void dummy(const char*) {}
+
+void invalid(const NotAString &s) {
+  dummy(s.c_str());
+}