[clang-tidy] Fix redundant-string-cstr check with msvc 14 headers.

[etienneb]

Summary

The string constructors are not defined using optional parameters and are not recognize by the checker.

The constructor defined in the MSVC header is defined with 1 parameter. Therefore, patterns are not recognized by the checker.
The current patch add support to accept constructor with only one parameter.

Repro on a Visual Studio 14 installation with the following code:

void f1(const std::string &s) {
  f1(s.c_str());
}

In the xstring.h header, the constructors are defined this way:

basic_string(const _Myt& _Right) [...]
basic_string(const _Myt& _Right, const _Alloc& _Al) [...]

The CXXConstructExpr to recognize only contains 1 parameter.

CXXConstructExpr 0x3f1a070 <C:\src\llvm\examples\test.cc:6:6, col:14> 'const std::string':'const class std::basic_string<char, struct std::char_traits<char>, class
 std::allocator<char> >' 'void (const char *) __attribute__((thiscall))'
`-CXXMemberCallExpr 0x3f1a008 <col:6, col:14> 'const char *'
  `-MemberExpr 0x3f19fe0 <col:6, col:8> '<bound member function type>' .c_str 0x3cc22f8
    `-DeclRefExpr 0x3f19fc8 <col:6> 'const std::string':'const class std::basic_string<char, struct std::char_traits<char>, class std::allocator<char> >' lvalue ParmVar 0x3f19c80 's' 'const std::string &'

[alexfh]

Looks good. Thank you!

Do you need me to submit the patch for you?

[etienneb]

rebased + fix invalid paths.

clang-tidy/readability/RedundantStringCStrCheck.cpp

 
 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;
 
-  Finder->addMatcher(
+  // Match string constructor.
+  const auto StringConstructorExpr = expr(anyOf(
+      cxxConstructExpr(
+          argumentCountIs(1),
+          hasDeclaration(cxxMethodDecl(hasName(StringConstructor)))),
       cxxConstructExpr(
-          hasDeclaration(cxxMethodDecl(hasName(StringConstructor))),
           argumentCountIs(2),
-          // The first argument must have the form x.c_str() or p->c_str()
-          // where the method is string::c_str().  We can use the copy
-          // constructor of string instead (or the compiler might share
-          // the string object).
-          hasArgument(0, cxxMemberCallExpr(
-                             callee(memberExpr().bind("member")),
-                             callee(cxxMethodDecl(hasName(StringCStrMethod))),
-                             on(expr().bind("arg")))
-                             .bind("call")),
-          // The second argument is the alloc object which must not be
-          // present explicitly.
-          hasArgument(1, cxxDefaultArgExpr())),
+          hasDeclaration(cxxMethodDecl(hasName(StringConstructor))),
+          // 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(
+                            on(expr().bind("arg"))).bind("call");
+
+  Finder->addMatcher(
+      cxxConstructExpr(StringConstructorExpr,
+                       hasArgument(0, StringCStrCallExpr)),
       this);
+
   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")))),
           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, cxxMemberCallExpr(
-                             callee(memberExpr().bind("member")),
-                             callee(cxxMethodDecl(hasName(StringCStrMethod))),
-                             on(expr().bind("arg")))
-                             .bind("call"))),
+          hasArgument(0, StringCStrCallExpr)),
       this);
 }
 
 void RedundantStringCStrCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Call = Result.Nodes.getStmtAs<CallExpr>("call");
   const auto *Arg = Result.Nodes.getStmtAs<Expr>("arg");
   bool Arrow = Result.Nodes.getStmtAs<MemberExpr>("member")->isArrow();
   // Replace the "call" node with the "arg" node, prefixed with '*'

test/clang-tidy/readability-redundant-string-cstr-msvc.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();
+  // MSVC headers define two constructors instead of using optional arguments.
+  basic_string(const C *p);
+  basic_string(const C *p, const A &a);
+  const C *c_str() const;
+};
+typedef basic_string<char, std::char_traits<char>, std::allocator<char>> string;
+}
+namespace llvm {
+struct StringRef {
+  StringRef(const char *p);
+  StringRef(const 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);{{$}}
+}