[PATCH] Refactor: Move remove-cstr-calls from a standalone executable to a clang-tidy check

[Richard]

================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:39
@@ -37,2 +38,3 @@
     CheckFactories.registerCheck<UseOverride>("misc-use-override");
+    CheckFactories.registerCheck<RemoveCStrCall>("misc-remove-cstr-call");
   }
----------------
alexfh wrote:
> I'd put this check under readability/ and call it "readability-redundant-cstr-calls".
Fixed.

================
Comment at: clang-tidy/misc/RemoveCStrCall.cpp:32
@@ +31,3 @@
+std::string getText(const SourceManager &SourceManager, const T &Node) {
+  SourceLocation StartSpellingLocation =
+      SourceManager.getSpellingLoc(Node.getLocStart());
----------------
LegalizeAdulthood wrote:
> alexfh wrote:
> > I suspect that most of this method can be replaced to a call to clang::Lexer::getSourceText (and this method can return StringRef).
> Diff in progress...
Fixed.

================
Comment at: clang-tidy/misc/RemoveCStrCall.cpp:84
@@ +83,3 @@
+                              const Expr &ExprNode) {
+  if (const clang::UnaryOperator *Op =
+      dyn_cast<clang::UnaryOperator>(&ExprNode)) {
----------------
alexfh wrote:
> Seems like a good use case for "auto".
Fixed.

================
Comment at: clang-tidy/misc/RemoveCStrCall.cpp:171
@@ +170,3 @@
+
+    diag(Call->getLocStart(), "redundant call to `c_str`.")
+            << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText);
----------------
alexfh wrote:
> nit: Please remove the trailing period.
> 
> I'd also use 'c_str()' instead of `c_str`.
Fixed

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:37
@@ +36,3 @@
+bool needParensAfterUnaryOperator(const Expr &ExprNode) {
+  if (dyn_cast<clang::BinaryOperator>(&ExprNode) ||
+      dyn_cast<clang::ConditionalOperator>(&ExprNode)) {
----------------
sbenza wrote:
> Use isa<> instead
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:23
@@ +22,3 @@
+template <typename T>
+StringRef getText(const ast_matchers::MatchFinder::MatchResult &Result, T const &Node) {
+    return Lexer::getSourceText(
----------------
alexfh wrote:
> clang-format?
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:50
@@ +49,3 @@
+// when it already begins with '&'.  Return empty string on failure.
+std::string formatDereference(const ast_matchers::MatchFinder::MatchResult &Result,
+                              const Expr &ExprNode) {
----------------
alexfh wrote:
> clang-format?
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:58
@@ +57,3 @@
+  }
+  const std::string Text = getText(Result, ExprNode);
+  if (Text.empty()) return std::string();
----------------
alexfh wrote:
> It's better to make this a `StringRef` and replace the concatenation of strings below with:
>       return (llvm::Twine("*(") + Text + ")").str().
Fixed

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:129
@@ +128,3 @@
+{
+    const CallExpr *Call = Result.Nodes.getStmtAs<CallExpr>("call");
+    const Expr *Arg = Result.Nodes.getStmtAs<Expr>("arg");
----------------
alexfh wrote:
> This is a good place to use `auto` or `const auto *`.
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:131
@@ +130,3 @@
+    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 '*'
----------------
alexfh wrote:
> s/const bool/bool/
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:134
@@ +133,3 @@
+    // if the call was using '->' rather than '.'.
+    const std::string ArgText = Arrow ?
+            formatDereference(Result, *Arg) :
----------------
alexfh wrote:
> s/const std::string/std::string/
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.cpp:136
@@ +135,3 @@
+            formatDereference(Result, *Arg) :
+            static_cast<std::string>(getText(Result, *Arg));
+    if (ArgText.empty()) return;
----------------
alexfh wrote:
> Why `static_cast` instead of `.str()`?
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.h:20
@@ +19,3 @@
+/// \brief Finds unnecessary calls to std::string::c_str().
+class RemoveCStrCall : public ClangTidyCheck {
+public:
----------------
alexfh wrote:
> 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.
Fixed.

================
Comment at: clang-tidy/readability/RemoveCStrCall.h:10-11
@@ +9,4 @@
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_REMOVE_C_STR_CALL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_REMOVE_C_STR_CALL_H
+
----------------
LegalizeAdulthood wrote:
> Oops, this still says misc
Fixed.

================
Comment at: test/clang-tidy/misc-remove-cstr-call.cpp:23
@@ +22,3 @@
+  f1(s.c_str());
+  // CHECK-FIXES: f1(s)
+}
----------------
alexfh wrote:
> Please make the check more specific to avoid incorrect matches:
>   // CHECK-FIXES: {{^  }}f1(s);{{$}}
Fixed.

================
Comment at: test/clang-tidy/misc-remove-cstr-call.cpp:28
@@ +27,3 @@
+  f2(s.c_str());
+  // CHECK: std::string s; 
+  // CHECK-NEXT: f2(s)
----------------
alexfh wrote:
> This should be CHECK-FIXES:. Please also add CHECK-MESSAGES to verify the error messages.
>   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant call ....
Fixed.

http://reviews.llvm.org/D7318

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/