Refactor: Move remove-cstr-calls from a standalone executable to a clang-tidy check readability-redundant-string-cstr

[LegalizeAdulthood]

Summary

Moved the files and moved the test inputs, adjusting for use within clang-tidy instead of as a standalone utility.

[klimek]

Test

[alexfh]

It looks like if you add reviewers and subscribers after creating a differential revision, Phabricator won't send notification mails to them (unless you add a comment afterwards). In any case, it makes sense double-checking that a message found its way to cfe-commits at least.

Thanks for the patch! A few comments inline.

clang-tidy/misc/MiscTidyModule.cpp

39 ↗

(On Diff #19104)

I'd put this check under readability/ and call it "readability-redundant-cstr-calls".

clang-tidy/misc/RemoveCStrCall.cpp

32 ↗

(On Diff #19104)

I suspect that most of this method can be replaced to a call to clang::Lexer::getSourceText (and this method can return StringRef).

84 ↗

(On Diff #19104)

Seems like a good use case for "auto".

171 ↗

(On Diff #19104)

nit: Please remove the trailing period.

I'd also use 'c_str()' instead of c_str.

test/clang-tidy/misc-remove-cstr-call.cpp

23 ↗

(On Diff #19104)

Please make the check more specific to avoid incorrect matches:

// CHECK-FIXES: {{^  }}f1(s);{{$}}

28 ↗

(On Diff #19104)

This should be CHECK-FIXES:. Please also add CHECK-MESSAGES to verify the error messages.

// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant call ....

[LegalizeAdulthood]

Thanks for the comments!

I'll see if I can get an updated version tonight.

[LegalizeAdulthood]

Corrected issues found in review.

[LegalizeAdulthood]

Use Lexer::getSourceText instead of SourceManager to get source text

[mclow.lists]

I don't have a problem with making this a clang-tidy module, but it was (also) meant as an example of how to write a refactoring tool.

[LegalizeAdulthood]

In D7318#125262, @mclow.lists wrote:

I don't have a problem with making this a clang-tidy module, but it was (also) meant as an example of how to write a refactoring tool.

Do we now have enough examples of refactoring tools that the example motivation is no longer a concern?

clang-tidy/misc/RemoveCStrCall.cpp

32 ↗

(On Diff #19104)

Diff in progress...

clang-tidy/readability/RemoveCStrCall.h

10–11 ↗

(On Diff #19867)

Oops, this still says misc

[alexfh]

In D7318#126692, @LegalizeAdulthood wrote:

In D7318#125262, @mclow.lists wrote:

I don't have a problem with making this a clang-tidy module, but it was (also) meant as an example of how to write a refactoring tool.

Do we now have enough examples of refactoring tools that the example motivation is no longer a concern?

I didn't find any references to remove-cstr-calls from docs, so I don't think the tool is in any sense special. There is a clang-rename which can be used as an example of a refactoring tool, and there is a dedicated tool-template. IMO, these should be enough.

[sbenza]

clang-tidy/readability/RemoveCStrCall.cpp

37 ↗

(On Diff #19873)

Use isa<> instead

[LegalizeAdulthood]

Use isa<> instead of dyn_cast<>.
Move the check from namespace clang::tidy into namespace clang::tidy::readability

[alexfh]

clang-tidy/readability/RemoveCStrCall.cpp

23 ↗

(On Diff #20937)

clang-format?

50 ↗

(On Diff #20937)

clang-format?

58 ↗

(On Diff #20937)

It's better to make this a StringRef and replace the concatenation of strings below with:

return (llvm::Twine("*(") + Text + ")").str().

129 ↗

(On Diff #20937)

This is a good place to use auto or const auto *.

131 ↗

(On Diff #20937)

s/const bool/bool/

134 ↗

(On Diff #20937)

s/const std::string/std::string/

136 ↗

(On Diff #20937)

Why static_cast instead of .str()?

clang-tidy/readability/RemoveCStrCall.h

20 ↗

(On Diff #20937)

The fact that it removes something doesn't make it much different from other checks, and it doesn't deserve a place in the name. Let's call this check "readability-redundant-cstr" or "readability-redundant-cstr-calls" instead (and the class RedundantCStrCheck or RedundantCStrCallsCheck).

I posted a relevant comment earlier, but it has fallen through the cracks.

[LegalizeAdulthood]

Run clang-format on the source files.
Use the name readability-redundant-cstr-call for the check.
Update types of locals as requested

[alexfh]

In D7318#133243, @LegalizeAdulthood wrote:

Run clang-format on the source files.
Use the name readability-redundant-cstr-call for the check.

Thanks for updating this. However could you use one of the alternatives I suggested? If you lean toward using 'call' in the name, let it be readability-redundant-cstr-calls and correspondingly RedundantCStrCallsCheck for the name of the class and the source files. Something seems wrong to me in using singular call in the name of this check, as it's supposed to find any number of them, not just one. Also note the Check suffix for the class name (and corresponding file names. It's kind of a naming convention for the checks.

Thanks.

For the reference, here's my previous comment:

The fact that it removes something doesn't make it much different from other checks, and it doesn't deserve a place in the name. Let's call this check "readability-redundant-cstr" or "readability-redundant-cstr-calls" instead (and the class RedundantCStrCheck or RedundantCStrCallsCheck).

[LegalizeAdulthood]

clang-tidy/misc/MiscTidyModule.cpp

39 ↗

(On Diff #19104)

Fixed.

clang-tidy/misc/RemoveCStrCall.cpp

32 ↗

(On Diff #19104)

Fixed.

84 ↗

(On Diff #19104)

Fixed.

171 ↗

(On Diff #19104)

Fixed

clang-tidy/readability/RemoveCStrCall.cpp

23 ↗

(On Diff #20937)

Fixed.

50 ↗

(On Diff #20937)

Fixed.

58 ↗

(On Diff #20937)

Fixed

129 ↗

(On Diff #20937)

Fixed.

131 ↗

(On Diff #20937)

Fixed.

134 ↗

(On Diff #20937)

Fixed.

136 ↗

(On Diff #20937)

Fixed.

37 ↗

(On Diff #19873)

Fixed.

clang-tidy/readability/RemoveCStrCall.h

20 ↗

(On Diff #20937)

Fixed.

10–11 ↗

(On Diff #19867)

Fixed.

test/clang-tidy/misc-remove-cstr-call.cpp

23 ↗

(On Diff #19104)

Fixed.

28 ↗

(On Diff #19104)

Fixed.

[LegalizeAdulthood]

In D7318#133461, @alexfh wrote:

In D7318#133243, @LegalizeAdulthood wrote:

Run clang-format on the source files.
Use the name readability-redundant-cstr-call for the check.

Thanks for updating this. However could you use one of the alternatives I suggested? If you lean toward using 'call' in the name, let it be readability-redundant-cstr-calls and correspondingly RedundantCStrCallsCheck for the name of the class and the source files. Something seems wrong to me in using singular call in the name of this check, as it's supposed to find any number of them, not just one. Also note the Check suffix for the class name (and corresponding file names. It's kind of a naming convention for the checks.

Thanks.

For the reference, here's my previous comment:

The fact that it removes something doesn't make it much different from other checks, and it doesn't deserve a place in the name. Let's call this check "readability-redundant-cstr" or "readability-redundant-cstr-calls" instead (and the class RedundantCStrCheck or RedundantCStrCallsCheck).

clang-tidy/readability/RemoveCStrCall.h

20 ↗

(On Diff #20937)

I was thinking about names; we have RedundantSmartPtrGet and check name readability-redundant-smartptr-get, a check for redundant calls to get() on a smartptr class.

Would that make this RedundantStringCStr and check name readability-redundant-string-cstr?

[alexfh]

clang-tidy/readability/RemoveCStrCall.h

20 ↗

(On Diff #20937)

I'd prefer the checks have the Check suffix in the class name. This convention is not strictly followed, but it probably should. The motivation is discussed in this review thread: http://reviews.llvm.org/D4189.

Thus it can be RedundantStringCStrCheck and the check name readability-redundant-string-cstr.

[LegalizeAdulthood]

clang-tidy/readability/RemoveCStrCall.h

20 ↗

(On Diff #20937)

Sounds good, I'll see if I can update that tonight. I did notice that not all checks had classes ending in Check. I'll create another changeset that fixes that.

[LegalizeAdulthood]

Rename class to RedundantStringCStrCheck

[LegalizeAdulthood]

Improve the naming to be more consistent with other checks:
RedundantStringCStrCheck
readability-redundant-string-cstr

[LegalizeAdulthood]

Align comments and include guard with changed names

[alexfh]

The code looks fine now, but if you don't mind, I'd ask you to fix a few more nits (mostly left from the original tool's code). Once done, I'd be happy to commit the patch for you.

Thanks!

clang-tidy/readability/RedundantStringCStrCheck.cpp

39

s/const auto/const auto*/

39

Use LLVM naming style: Op

52

s/const auto/const auto*/

68

const char StringConstructor[] = ...

72

ditto

93

id is an old-style construct, now we prefer using .bind():

id("...", X) -> X.bind("...")

96

ditto

117

ditto

120

ditto

125

const auto*

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

25

nit: The number of spaces here should remain the same, so I'd better replace {{^ *}} with {{^ }}.

[LegalizeAdulthood]

clang-tidy/readability/RedundantStringCStrCheck.cpp

39

Fixed

52

Fixed

68

Fixed

72

Fixed

93

Fixed

96

Fixed

117

Fixed

120

Fixed

125

Fixed

[LegalizeAdulthood]

Prefer bind() over id()
Change types to be more consistent with llvm/clang
Chang header guard to be more consistent with llvm/clang

[LegalizeAdulthood]

Make test cases more specific.

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

25

Fixed

[alexfh]

Thank you!

Committed revision 232338.

CMakeLists.txt

 add_subdirectory(clang-apply-replacements)
 add_subdirectory(clang-modernize)
 add_subdirectory(clang-rename)
 add_subdirectory(modularize)
-add_subdirectory(remove-cstr-calls)
 if(CLANG_ENABLE_STATIC_ANALYZER)
 add_subdirectory(clang-tidy)
 endif()
 
 add_subdirectory(clang-query)
 add_subdirectory(pp-trace)
 add_subdirectory(tool-template)
 
 # Add the common testsuite after all the tools.
 # TODO: Support tests with more granularity when features are off?
 if(CLANG_ENABLE_STATIC_ANALYZER)
 add_subdirectory(test)
 add_subdirectory(unittests)
 endif()

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
+  RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   ShrinkToFitCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "RedundantSmartptrGetCheck.h"
+#include "RedundantStringCStrCheck.h"
 #include "ShrinkToFitCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");
-    CheckFactories.registerCheck<ShrinkToFitCheck>(
-        "readability-shrink-to-fit");
+    CheckFactories.registerCheck<RedundantStringCStrCheck>(
+        "readability-redundant-string-cstr");
+    CheckFactories.registerCheck<ShrinkToFitCheck>("readability-shrink-to-fit");
   }
 };
 
-} // namespace readability
-
 // Register the MiscTidyModule using this statically initialized variable.
-static ClangTidyModuleRegistry::Add<readability::ReadabilityModule>
+static ClangTidyModuleRegistry::Add<ReadabilityModule>
     X("readability-module", "Adds readability-related checks.");
 
+} // namespace readability
+
 // This anchor is used to force the linker to link in the generated object file
-// and thus register the MiscModule.
+// and thus register the ReadabilityModule.
 volatile int ReadabilityModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/readability/RedundantStringCStrCheck.h

+//===--- RedundantStringCStrCheck.h - clang-tidy ----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTSTRINGCSTRCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTSTRINGCSTRCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// \brief Finds unnecessary calls to std::string::c_str().
+class RedundantStringCStrCheck : public ClangTidyCheck {
+public:
+  RedundantStringCStrCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTSTRINGCSTRCHECK_H

clang-tidy/readability/RedundantStringCStrCheck.cpp

+//===- RedundantStringCStrCheck.cpp - Check for redundant c_str call ------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file implements a check for redundant calls of c_str() on strings.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantStringCStrCheck.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+
+namespace {
+
+template <typename T>
+StringRef getText(const ast_matchers::MatchFinder::MatchResult &Result,
+                  T const &Node) {
+  return Lexer::getSourceText(
+      CharSourceRange::getTokenRange(Node.getSourceRange()),
+      *Result.SourceManager, Result.Context->getLangOpts());
+}
+
+// Return true if expr needs to be put in parens when it is an argument of a
+// prefix unary operator, e.g. when it is a binary or ternary operator
+// syntactically.
+bool needParensAfterUnaryOperator(const Expr &ExprNode) {
+  if (isa<clang::BinaryOperator>(&ExprNode) ||
+      isa<clang::ConditionalOperator>(&ExprNode)) {
+    return true;
+  }
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(&ExprNode)) {
+    return Op->getNumArgs() == 2 && Op->getOperator() != OO_PlusPlus &&
+           Op->getOperator() != OO_MinusMinus && Op->getOperator() != OO_Call &&
+           Op->getOperator() != OO_Subscript;
+  }
+  return false;
+}
+
+// Format a pointer to an expression: prefix with '*' but simplify
+// when it already begins with '&'.  Return empty string on failure.
+std::string
+formatDereference(const ast_matchers::MatchFinder::MatchResult &Result,
+                  const Expr &ExprNode) {
+  if (const auto *Op = dyn_cast<clang::UnaryOperator>(&ExprNode)) {
+    if (Op->getOpcode() == UO_AddrOf) {
+      // Strip leading '&'.
+      return getText(Result, *Op->getSubExpr()->IgnoreParens());
+    }
+  }
+  StringRef Text = getText(Result, ExprNode);
+  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 clang {
+namespace tidy {
+namespace readability {
+
+void RedundantStringCStrCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+      constructExpr(
+          hasDeclaration(methodDecl(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, memberCallExpr(callee(memberExpr().bind("member")),
+                                callee(methodDecl(hasName(StringCStrMethod))),
+                                on(expr().bind("arg"))).bind("call")),
+          // The second argument is the alloc object which must not be
+          // present explicitly.
+          hasArgument(1, defaultArgExpr())),
+      this);
+  Finder->addMatcher(
+      constructExpr(
+          // 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(
+              methodDecl(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, memberCallExpr(callee(memberExpr().bind("member")),
+                                callee(methodDecl(hasName(StringCStrMethod))),
+                                on(expr().bind("arg"))).bind("call"))),
+      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 '*'
+  // 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()`")
+      << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

remove-cstr-calls/CMakeLists.txt

-set(LLVM_LINK_COMPONENTS
-  Support
-  )
-
-add_clang_executable(remove-cstr-calls
-  RemoveCStrCalls.cpp
-  )
-
-target_link_libraries(remove-cstr-calls
-  clangAST
-  clangASTMatchers
-  clangBasic
-  clangFrontend
-  clangLex
-  clangTooling
-  clangToolingCore
-  )

remove-cstr-calls/Makefile

-##===- tools/extra/remove-cstr-calls/Makefile --------------*- Makefile -*-===##
-#
-#                     The LLVM Compiler Infrastructure
-#
-# This file is distributed under the University of Illinois Open Source
-# License. See LICENSE.TXT for details.
-#
-##===----------------------------------------------------------------------===##
-
-CLANG_LEVEL := ../../..
-
-TOOLNAME = remove-cstr-calls
-NO_INSTALL = 1
-
-# No plugins, optimize startup time.
-TOOL_NO_EXPORTS = 1
-
-include $(CLANG_LEVEL)/../../Makefile.config
-LINK_COMPONENTS := $(TARGETS_TO_BUILD) asmparser bitreader support mc option
-USEDLIBS = clangTooling.a clangFrontend.a clangSerialization.a clangDriver.a \
-           clangToolingCore.a clangRewriteFrontend.a clangRewrite.a \
-           clangParse.a clangSema.a clangAnalysis.a \
-           clangAST.a clangASTMatchers.a clangEdit.a clangLex.a clangBasic.a
-
-include $(CLANG_LEVEL)/Makefile

remove-cstr-calls/RemoveCStrCalls.cpp

-//===- examples/Tooling/RemoveCStrCalls.cpp - Redundant c_str call removal ===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-//  This file implements a tool that prints replacements that remove redundant
-//  calls of c_str() on strings.
-//
-//  Usage:
-//  remove-cstr-calls <cmake-output-dir> <file1> <file2> ...
-//
-//  Where <cmake-output-dir> is a CMake build directory in which a file named
-//  compile_commands.json exists (enable -DCMAKE_EXPORT_COMPILE_COMMANDS in
-//  CMake to get this output).
-//
-//  <file1> ... specify the paths of files in the CMake source tree. This path
-//  is looked up in the compile command database. If the path of a file is
-//  absolute, it needs to point into CMake's source tree. If the path is
-//  relative, the current working directory needs to be in the CMake source
-//  tree and the file must be in a subdirectory of the current working
-//  directory. "./" prefixes in the relative files will be automatically
-//  removed, but the rest of a relative path must be a suffix of a path in
-//  the compile command line database.
-//
-//  For example, to use remove-cstr-calls on all files in a subtree of the
-//  source tree, use:
-//
-//    /path/in/subtree $ find . -name '*.cpp'|
-//        xargs remove-cstr-calls /path/to/build
-//
-//===----------------------------------------------------------------------===//
-
-#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Basic/SourceManager.h"
-#include "clang/Frontend/FrontendActions.h"
-#include "clang/Lex/Lexer.h"
-#include "clang/Tooling/CompilationDatabase.h"
-#include "clang/Tooling/Refactoring.h"
-#include "clang/Tooling/Tooling.h"
-#include "llvm/ADT/Twine.h"
-#include "llvm/Support/CommandLine.h"
-#include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Path.h"
-#include "llvm/Support/Signals.h"
-#include "llvm/Support/raw_ostream.h"
-#include <system_error>
-
-using namespace clang;
-using namespace clang::ast_matchers;
-using namespace llvm;
-using clang::tooling::newFrontendActionFactory;
-using clang::tooling::Replacement;
-using clang::tooling::CompilationDatabase;
-
-// FIXME: Pull out helper methods in here into more fitting places.
-
-// Returns the text that makes up 'node' in the source.
-// Returns an empty string if the text cannot be found.
-template <typename T>
-static std::string getText(const SourceManager &SourceManager, const T &Node) {
-  SourceLocation StartSpellingLocation =
-      SourceManager.getSpellingLoc(Node.getLocStart());
-  SourceLocation EndSpellingLocation =
-      SourceManager.getSpellingLoc(Node.getLocEnd());
-  if (!StartSpellingLocation.isValid() || !EndSpellingLocation.isValid()) {
-    return std::string();
-  }
-  bool Invalid = true;
-  const char *Text =
-      SourceManager.getCharacterData(StartSpellingLocation, &Invalid);
-  if (Invalid) {
-    return std::string();
-  }
-  std::pair<FileID, unsigned> Start =
-      SourceManager.getDecomposedLoc(StartSpellingLocation);
-  std::pair<FileID, unsigned> End =
-      SourceManager.getDecomposedLoc(Lexer::getLocForEndOfToken(
-          EndSpellingLocation, 0, SourceManager, LangOptions()));
-  if (Start.first != End.first) {
-    // Start and end are in different files.
-    return std::string();
-  }
-  if (End.second < Start.second) {
-    // Shuffling text with macros may cause this.
-    return std::string();
-  }
-  return std::string(Text, End.second - Start.second);
-}
-
-// Return true if expr needs to be put in parens when it is an argument of a
-// prefix unary operator, e.g. when it is a binary or ternary operator
-// syntactically.
-static bool needParensAfterUnaryOperator(const Expr &ExprNode) {
-  if (dyn_cast<clang::BinaryOperator>(&ExprNode) ||
-      dyn_cast<clang::ConditionalOperator>(&ExprNode)) {
-    return true;
-  }
-  if (const CXXOperatorCallExpr *op =
-      dyn_cast<CXXOperatorCallExpr>(&ExprNode)) {
-    return op->getNumArgs() == 2 &&
-        op->getOperator() != OO_PlusPlus &&
-        op->getOperator() != OO_MinusMinus &&
-        op->getOperator() != OO_Call &&
-        op->getOperator() != OO_Subscript;
-  }
-  return false;
-}
-
-// Format a pointer to an expression: prefix with '*' but simplify
-// when it already begins with '&'.  Return empty string on failure.
-static std::string formatDereference(const SourceManager &SourceManager,
-                              const Expr &ExprNode) {
-  if (const clang::UnaryOperator *Op =
-      dyn_cast<clang::UnaryOperator>(&ExprNode)) {
-    if (Op->getOpcode() == UO_AddrOf) {
-      // Strip leading '&'.
-      return getText(SourceManager, *Op->getSubExpr()->IgnoreParens());
-    }
-  }
-  const std::string Text = getText(SourceManager, ExprNode);
-  if (Text.empty()) return std::string();
-  // Add leading '*'.
-  if (needParensAfterUnaryOperator(ExprNode)) {
-    return std::string("*(") + Text + ")";
-  }
-  return std::string("*") + Text;
-}
-
-namespace {
-class FixCStrCall : public ast_matchers::MatchFinder::MatchCallback {
- public:
-  FixCStrCall(tooling::Replacements *Replace)
-      : Replace(Replace) {}
-
-  virtual void run(const ast_matchers::MatchFinder::MatchResult &Result) {
-    const CallExpr *Call =
-        Result.Nodes.getStmtAs<CallExpr>("call");
-    const Expr *Arg =
-        Result.Nodes.getStmtAs<Expr>("arg");
-    const bool Arrow =
-        Result.Nodes.getStmtAs<MemberExpr>("member")->isArrow();
-    // Replace the "call" node with the "arg" node, prefixed with '*'
-    // if the call was using '->' rather than '.'.
-    const std::string ArgText = Arrow ?
-        formatDereference(*Result.SourceManager, *Arg) :
-        getText(*Result.SourceManager, *Arg);
-    if (ArgText.empty()) return;
-
-    Replace->insert(Replacement(*Result.SourceManager, Call, ArgText));
-  }
-
- private:
-  tooling::Replacements *Replace;
-};
-} // end namespace
-
-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";
-
-cl::opt<std::string> BuildPath(
-  cl::Positional,
-  cl::desc("<build-path>"));
-
-cl::list<std::string> SourcePaths(
-  cl::Positional,
-  cl::desc("<source0> [... <sourceN>]"),
-  cl::OneOrMore);
-
-int main(int argc, const char **argv) {
-  llvm::sys::PrintStackTraceOnErrorSignal();
-  std::unique_ptr<CompilationDatabase> Compilations(
-      tooling::FixedCompilationDatabase::loadFromCommandLine(argc, argv));
-  cl::ParseCommandLineOptions(argc, argv);
-  if (!Compilations) {
-    std::string ErrorMessage;
-    Compilations =
-        CompilationDatabase::loadFromDirectory(BuildPath, ErrorMessage);
-    if (!Compilations)
-      llvm::report_fatal_error(ErrorMessage);
-  }
-  tooling::RefactoringTool Tool(*Compilations, SourcePaths);
-  ast_matchers::MatchFinder Finder;
-  FixCStrCall Callback(&Tool.getReplacements());
-  Finder.addMatcher(
-      constructExpr(
-          hasDeclaration(methodDecl(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,
-              id("call", memberCallExpr(
-                  callee(id("member", memberExpr())),
-                  callee(methodDecl(hasName(StringCStrMethod))),
-                  on(id("arg", expr()))))),
-          // The second argument is the alloc object which must not be
-          // present explicitly.
-          hasArgument(
-              1,
-              defaultArgExpr())),
-      &Callback);
-  Finder.addMatcher(
-      constructExpr(
-          // 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(methodDecl(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,
-              id("call", memberCallExpr(
-                  callee(id("member", memberExpr())),
-                  callee(methodDecl(hasName(StringCStrMethod))),
-                  on(id("arg", expr())))))),
-      &Callback);
-  return Tool.runAndSave(newFrontendActionFactory(&Finder).get());
-}

test/CMakeLists.txt

set(CLANG_TOOLS_TEST_DEPS
   # Individual tools we test.
   clang-apply-replacements
   clang-modernize
   clang-rename
   clang-query
   clang-tidy
   modularize
   pp-trace
-  remove-cstr-calls
 
   # Unit tests
   ExtraToolsUnitTests
   )
 
 add_lit_testsuite(check-clang-tools "Running the Clang extra tools' regression tests"
   ${CMAKE_CURRENT_BINARY_DIR}
   DEPENDS ${CLANG_TOOLS_TEST_DEPS}
   ARGS ${CLANG_TOOLS_TEST_EXTRA_ARGS}
   )
 set_target_properties(check-clang-tools PROPERTIES FOLDER "Clang extra tools' tests")
 

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

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-redundant-string-cstr %t
+// REQUIRES: shell
+
+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;
+}
+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);{{$}}
+}

test/remove-cstr-calls/basic.cpp

-// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: remove-cstr-calls . %t.cpp --
-// RUN: FileCheck -input-file=%t.cpp %s
-// REQUIRES: shell
-
-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;
-}
-namespace llvm {
-struct StringRef {
-  StringRef(const char *p);
-  StringRef(const std::string &);
-};
-}
-
-void f1(const std::string &s) {
-  f1(s.c_str());
-  // CHECK: void f1
-  // CHECK-NEXT: f1(s)
-}
-void f2(const llvm::StringRef r) {
-  std::string s;
-  f2(s.c_str());
-  // CHECK: std::string s; 
-  // CHECK-NEXT: f2(s)
-}
-void f3(const llvm::StringRef &r) {
-  std::string s;
-  f3(s.c_str());
-  // CHECK: std::string s;
-  // CHECK: f3(s)
-}