[clang-tidy] Add more detection rules for redundant c_str calls.

[etienneb]

Summary

The string class contains methods which support receiving either a string literal or a string object.

For example, calls to append can receive either a char* or a string.

string& append (const string& str);
string& append (const char* s);

Which make these cases equivalent, and the .c_str() useless:

std::string s = "123";
str.append(s);
str.append(s.c_str());

In these cases, removing .c_str() doesn't provide any size or speed improvement.
It's only a readability issue.

If the string contains embedded NUL characters, the string literal and the string
object won't produce the same semantic.

[Eugene.Zelenko]

Thank you for enhancement!

I think will be good idea to extend check for any function call that receives .c_str(), since it mat be in other classes could have methods which receives both C++ or C strings. See also PR24870.

[etienneb]

In D18475#383520, @Eugene.Zelenko wrote:

Thank you for enhancement!

I think will be good idea to extend check for any function call that receives .c_str(), since it mat be in other classes could have methods which receives both C++ or C strings. See also PR24870.

I was thinking the same when I started that patch. But, there is multiple cases where it will bring a false-positive.
As an example:

string& insert (size_t pos, const string& str, size_t subpos, size_t sublen = npos);
string& insert (size_t pos, const char* s, size_t n);

Even if the overloaded name/types are compatible, the semantic is not the same. You can't simply remove the .c_str() here.

I agree that a more general cases could exists, but I would like to be able to turn it off over a large code base when trying to keep false positives as low as posible.

[etienneb]

Fixing some matchers to use the recent new AstMatcher.
(see http://reviews.llvm.org/D18275)

[etienneb]

chapuni@: I would like to know why "-target x86_64-unknown -std=c++11" was needed.

I'm fixing an issue with char16_t/char32_t. Is that the problem you just fixed?

[etienneb]

fix unittest on linux/32-bits.
tested on windows/32-bits.

[aaron.ballman]

clang-tidy/readability/RedundantStringCStrCheck.cpp

129

I think this comment is incorrect, you want assignment, not equality.

[etienneb]

fix invalid comment.

[etienneb]

thanks, aaron.ballman@.

clang-tidy/readability/RedundantStringCStrCheck.cpp

129

copy-paste! Good catch.

[alexfh]

Looks good to me. Adding Samuel, since he has done a lot of similar stuff by now and might have good ideas on improving this and making this more general.

[sbenza]

In D18475#385789, @alexfh wrote:

Looks good to me. Adding Samuel, since he has done a lot of similar stuff by now and might have good ideas on improving this and making this more general.

There are two things I've done before that might apply here.
First, you can move the function+arg into data and just loop through it to make the matchers.
Eg:

struct Case {
  StringRef Func;
  unsigned Arg;
};
const Case Cases[] = {...};
for (Case C : Cases) {
  Finder->addMatcher(...);
}

This reduces the amount of code duplication.

Another thing I've done is to find matching overloads dynamically instead of hardcoding the list of functions.
The idea is that you match something like callExpr(hasAnyArgument(StringCStrCallExpr)) and then you figure out if that function has a matching overload that takes const string& on that specific argument.
This is flexible, but might give false positives.

[etienneb]

I'll to try to make a better way to describe redundant cases (as proposed by sbenza@) with an array.
I like the array based approach, it's lean and clean.

I'll try to make a prototype of the generic overloaded functions matcher and I'll look at the ratio of false-positive.
It may be gated under a configuration to avoid reporting these false-positives.

Afterward, we can take decision of what/how should this be integrated.

[LegalizeAdulthood]

Can you summarize the improvements in docs/ReleaseNotes.rst please?

clang-tidy/readability/RedundantStringCStrCheck.cpp

void RedundantStringCStrCheck::registerMatchers(
 
   // Match a call to the string 'c_str()' method.
   const auto StringCStrCallExpr =
       cxxMemberCallExpr(on(StringExpr.bind("arg")),
                         callee(memberExpr().bind("member")),
                         callee(cxxMethodDecl(hasName("c_str"))))
           .bind("call");
 
+  // Detect redundant 'c_str()' calls through a string constructor.
   Finder->addMatcher(
       cxxConstructExpr(StringConstructorExpr,
                        hasArgument(0, StringCStrCallExpr)),
       this);
 
+  // Detect: 's == str.c_str()'  ->  's == str'
+  Finder->addMatcher(
+      cxxOperatorCallExpr(
+          anyOf(hasOverloadedOperatorName("<"),
+                hasOverloadedOperatorName(">"),
+                hasOverloadedOperatorName(">="),
+                hasOverloadedOperatorName("<="),
+                hasOverloadedOperatorName("!="),
+                hasOverloadedOperatorName("=="),
+                hasOverloadedOperatorName("+")),
+          anyOf(allOf(hasArgument(0, StringExpr),
+                      hasArgument(1, StringCStrCallExpr)),
+                allOf(hasArgument(0, StringCStrCallExpr),
+                      hasArgument(1, StringExpr)))),
+      this);
+
+  // Detect: 'dst += str.c_str()'  ->  'dst += str'
+  // Detect: 's == str.c_str()'  ->  's == str'
+  Finder->addMatcher(
+      cxxOperatorCallExpr(
+          anyOf(hasOverloadedOperatorName("="),
+                hasOverloadedOperatorName("+=")),
+          hasArgument(0, StringExpr),
+          hasArgument(1, StringCStrCallExpr)),
+      this);
+
+  // Detect: 'dst.append(str.c_str())'  ->  'dst.append(str)'
+  Finder->addMatcher(
+      cxxMemberCallExpr(on(StringExpr),
+          callee(decl(cxxMethodDecl(
+              hasAnyName("append", "assign", "compare")))),
+          argumentCountIs(1),
+          hasArgument(0, StringCStrCallExpr)),
+      this);
+
+  // Detect: 'dst.compare(p, n, str.c_str())'  ->  'dst.compare(p, n, str)'
+  Finder->addMatcher(
+      cxxMemberCallExpr(on(StringExpr),
+          callee(decl(cxxMethodDecl(hasName("compare")))),
+          argumentCountIs(3),
+          hasArgument(2, StringCStrCallExpr)),
+      this);
+
+  // Detect: 'dst.find(str.c_str())'  ->  'dst.find(str)'
+  Finder->addMatcher(
+      cxxMemberCallExpr(on(StringExpr),
+          callee(decl(cxxMethodDecl(
+              hasAnyName("find", "find_first_not_of", "find_first_of",
+                         "find_last_not_of", "find_last_of", "rfind")))),
+          anyOf(argumentCountIs(1), argumentCountIs(2)),
+          hasArgument(0, StringCStrCallExpr)),
+      this);
+
+  // Detect: 'dst.insert(pos, str.c_str())'  ->  'dst.insert(pos, str)'
+  Finder->addMatcher(
+      cxxMemberCallExpr(on(StringExpr),
+          callee(decl(cxxMethodDecl(hasName("insert")))),
+          argumentCountIs(2),
+          hasArgument(1, StringCStrCallExpr)),
+      this);
+
+  // Detect redundant 'c_str()' calls through a StringRef constructor.
   Finder->addMatcher(
       cxxConstructExpr(
           // Implicit constructors of these classes are overloaded
           // wrt. string types and they internally make a StringRef
           // referring to the argument.  Passing a string directly to
           // them is preferred to passing a char pointer.
           hasDeclaration(
-              cxxMethodDecl(anyOf(hasName("::llvm::StringRef::StringRef"),
-                                  hasName("::llvm::Twine::Twine")))),
+              cxxMethodDecl(hasAnyName("::llvm::StringRef::StringRef",
+                                       "::llvm::Twine::Twine"))),
           argumentCountIs(1),
           // 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);

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

-// RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- -target x86_64-unknown -std=c++11
+// RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- -std=c++11
+
+typedef unsigned __INT16_TYPE__ char16;
+typedef unsigned __INT32_TYPE__ char32;
+typedef __SIZE_TYPE__ size;
 
 namespace std {
 template <typename T>
 class allocator {};
 template <typename T>
 class char_traits {};
 template <typename C, typename T, typename A>
 struct basic_string {
+  typedef basic_string<C, T, A> _Type;
   basic_string();
   basic_string(const C *p, const A &a = A());
+
   const C *c_str() const;
+
+  _Type& append(const C *s);
+  _Type& append(const C *s, size n);
+  _Type& assign(const C *s);
+  _Type& assign(const C *s, size n);
+
+  int compare(const _Type&) const;
+  int compare(const C* s) const;
+  int compare(size pos, size len, const _Type&) const;
+  int compare(size pos, size len, const C* s) const;
+
+  size find(const _Type& str, size pos = 0) const;
+  size find(const C* s, size pos = 0) const;
+  size find(const C* s, size pos, size n) const;
+
+  _Type& insert(size pos, const _Type& str);
+  _Type& insert(size pos, const C* s);
+  _Type& insert(size pos, const C* s, size n);
+
+  _Type& operator+=(const _Type& str);
+  _Type& operator+=(const C* s);
+  _Type& operator=(const _Type& str);
+  _Type& operator=(const C* s);
 };
+
 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;
+typedef basic_string<char16, std::char_traits<char16>, std::allocator<char16>> u16string;
+typedef basic_string<char32, std::char_traits<char32>, std::allocator<char32>> u32string;
 }
 
+std::string operator+(const std::string&, const std::string&);
+std::string operator+(const std::string&, const char*);
+std::string operator+(const char*, const std::string&);
+
+bool operator==(const std::string&, const std::string&);
+bool operator==(const std::string&, const char*);
+bool operator==(const char*, const std::string&);
+
 namespace llvm {
 struct StringRef {
   StringRef(const char *p);
   StringRef(const std::string &);
 };
 }
 
 // Tests for std::string.
void f3(const llvm::StringRef &r) {
   // 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);{{$}}
 }
+void f5(const std::string &s) {
+  std::string tmp;
+  tmp.append(s.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}tmp.append(s);{{$}}
+  tmp.assign(s.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}tmp.assign(s);{{$}}
+
+  if (tmp.compare(s.c_str()) == 0) return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}if (tmp.compare(s) == 0) return;{{$}}
+
+  if (tmp.compare(1, 2, s.c_str()) == 0) return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}if (tmp.compare(1, 2, s) == 0) return;{{$}}
+
+  if (tmp.find(s.c_str()) == 0) return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}if (tmp.find(s) == 0) return;{{$}}
+
+  if (tmp.find(s.c_str(), 2) == 0) return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}if (tmp.find(s, 2) == 0) return;{{$}}
+
+  if (tmp.find(s.c_str(), 2) == 0) return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}if (tmp.find(s, 2) == 0) return;{{$}}
+
+  tmp.insert(1, s.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}tmp.insert(1, s);{{$}}
+
+  tmp = s.c_str();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}tmp = s;{{$}}
+
+  tmp += s.c_str();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}tmp += s;{{$}}
+
+  if (tmp == s.c_str()) return;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}if (tmp == s) return;{{$}}
+
+  tmp = s + s.c_str();
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}tmp = s + s;{{$}}
+
+  tmp = s.c_str() + s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant call {{.*}}
+  // CHECK-FIXES: {{^  }}tmp = s + s;{{$}}
+}
+void f6(const std::string &s) {
+  std::string tmp;
+  tmp.append(s.c_str(), 2);
+  tmp.assign(s.c_str(), 2);
+
+  if (tmp.compare(s) == 0) return;
+  if (tmp.compare(1, 2, s) == 0) return;
+
+  tmp = s;
+  tmp += s;
+
+  if (tmp == s)
+    return;
+
+  tmp = s + s;
+
+  if (tmp.find(s.c_str(), 2, 4) == 0) return;
+
+  tmp.insert(1, s);
+  tmp.insert(1, s.c_str(), 2);
+}
 
 // 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);{{$}}
 }