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

[Alexander Kornienko]

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.


================
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");
   }
----------------
I'd put this check under readability/ and call it "readability-redundant-cstr-calls".

================
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());
----------------
I suspect that most of this method can be replaced to a call to clang::Lexer::getSourceText (and this method can return StringRef).

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

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

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

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

================
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)
----------------
This should be CHECK-FIXES:. Please also add CHECK-MESSAGES to verify the error messages.
  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant call ....

http://reviews.llvm.org/D7318

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