[clang-tidy] Handle data() in readability-redundant-string-cstr

[malcolm.parsons]

Summary

std::string::data() and std::string::c_str() are equivalent.
Enhance the readability-redundant-string-cstr check to also handle
calls to data().

[aaron.ballman]

LGTM!

I like the idea, but would point out that data() and c_str() had different semantics once upon a time. In c++03 and earlier, data() was not guaranteed to be null terminated, but`c_str()` was. However, I'm not certain that's worth altering the behavior of this check for; I don't think any implementation actually made that distinction in practice.

[malcolm.parsons]

In D26279#586641, @aaron.ballman wrote:

I like the idea, but would point out that data() and c_str() had different semantics once upon a time.

I'm aware of this, but it doesn't affect whether the call is redundant.

[malcolm.parsons]

Add to release notes.
Add to documentation of check.

clang-tidy/readability/RedundantStringCStrCheck.cpp

const auto StringConstructorExpr = expr(anyOf(
           // 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(StringExpr.bind("arg")),
                         callee(memberExpr().bind("member")),
-                        callee(cxxMethodDecl(hasName("c_str"))))
+                        callee(cxxMethodDecl(hasAnyName("c_str", "data"))))
           .bind("call");
 
   // Detect redundant 'c_str()' calls through a string constructor.
   Finder->addMatcher(
       cxxConstructExpr(StringConstructorExpr,
                        hasArgument(0, StringCStrCallExpr)),
       this);
 
Finder->addMatcher(
           // directly.
           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();
+  const auto *Member = Result.Nodes.getStmtAs<MemberExpr>("member");
+  bool Arrow = Member->isArrow();
   // Replace the "call" node with the "arg" node, prefixed with '*'
   // if the call was using '->' rather than '.'.
   std::string ArgText =
       Arrow ? formatDereference(Result, *Arg) : getText(Result, *Arg).str();
   if (ArgText.empty())
     return;
 
-  diag(Call->getLocStart(), "redundant call to `c_str()`")
+  diag(Call->getLocStart(), "redundant call to %0")
+      << Member->getMemberDecl()
       << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

- New `readability-redundant-declaration
   Finds redundant variable and function declarations.
 
 - New `readability-redundant-member-init
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-member-init.html>`_ check
 
   Flags member initializations that are unnecessary because the same default
   constructor would be called if they were not present.
 
+- The `readability-redundant-string-cstr
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-cstr.html>`_ check
+  now warns about redundant calls to data() too.
+
 Fixed bugs:
 
 - `modernize-make-unique
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-unique.html>`_
   and `modernize-make-shared
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html>`_
   Calling ``make_{unique|shared}`` from within a member function of a type
   with a private or protected constructor would be ill-formed.

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()``.
+Finds unnecessary calls to ``std::string::c_str()`` and ``std::string::data()``.

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;
+  const C *data() 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-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}f1(s);{{$}}
+  f1(s.data());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to 'data' [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);{{$}}
 }

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

 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;
+  const C *data() 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;
struct StringRef {
   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-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}f1(s);{{$}}
+  f1(s.data());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant call to 'data' [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-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());
void f6(const std::string &s) {
   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-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-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-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());
 }