Add handling for logging and gtest comparison macros.

tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp

Index: tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp
diff --git a/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp b/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp
index a5394bf188ebf9b667e62b1daf627f84c763461e..27e5b7fab07864debb7b4bfcea3aef53b7d2bb5b 100644
--- a/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp
+++ b/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp
@@ -6,6 +6,7 @@
 // scoped_refptr<T>'s implicit cast to T (operator T*) to an explicit call to
 // the .get() method.
 
+#include <assert.h>
 #include <algorithm>
 #include <memory>
 #include <string>
@@ -61,54 +62,20 @@ bool NeedsParens(const clang::Expr* expr) {
   return false;
 }
 
-Replacement RewriteRawPtrToScopedRefptr(const MatchFinder::MatchResult& result,
-                                        clang::SourceLocation begin,
-                                        clang::SourceLocation end) {
-  clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(
-      result.SourceManager->getSpellingLoc(begin),
-      result.SourceManager->getSpellingLoc(end));
-
-  std::string text = clang::Lexer::getSourceText(
-      range, *result.SourceManager, result.Context->getLangOpts());
-  text.erase(text.rfind('*'));
-
-  std::string replacement_text("scoped_refptr<");
-  replacement_text += text;
-  replacement_text += ">";
-
-  return Replacement(*result.SourceManager, range, replacement_text);
-}
-
-class GetRewriterCallback : public MatchFinder::MatchCallback {
- public:
-  explicit GetRewriterCallback(Replacements* replacements)
-      : replacements_(replacements) {}
-  virtual void run(const MatchFinder::MatchResult& result) override;
-
- private:
-  Replacements* const replacements_;
-};
-
-void GetRewriterCallback::run(const MatchFinder::MatchResult& result) {
-  const clang::CXXMemberCallExpr* const implicit_call =
-      result.Nodes.getNodeAs<clang::CXXMemberCallExpr>("call");
-  const clang::Expr* arg = result.Nodes.getNodeAs<clang::Expr>("arg");
-
-  if (!implicit_call || !arg)
-    return;
-
+Replacement RewriteImplicitToExplicitConversion(
+    const MatchFinder::MatchResult& result,
+    const clang::Expr* expr) {
   clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(
-      result.SourceManager->getSpellingLoc(arg->getLocStart()),
-      result.SourceManager->getSpellingLoc(arg->getLocEnd()));
-  if (!range.isValid())
-    return;  // TODO(rsleevi): Log an error?
+      result.SourceManager->getSpellingLoc(expr->getLocStart()),
+      result.SourceManager->getSpellingLoc(expr->getLocEnd()));
+  assert(range.isValid() && "Invalid range!");

[dcheng, 2014/08/20 07:02:30]

This is apparently LLVM style.

 
   // Handle cases where an implicit cast is being done by dereferencing a
   // pointer to a scoped_refptr<> (sadly, it happens...)
   //
   // This rewrites both "*foo" and "*(foo)" as "foo->get()".
   if (const clang::UnaryOperator* op =
-          llvm::dyn_cast<clang::UnaryOperator>(arg)) {
+          llvm::dyn_cast<clang::UnaryOperator>(expr)) {
     if (op->getOpcode() == clang::UO_Deref) {
       const clang::Expr* const sub_expr =
           op->getSubExpr()->IgnoreParenImpCasts();
@@ -116,41 +83,36 @@ void GetRewriterCallback::run(const MatchFinder::MatchResult& result) {
           clang::CharSourceRange::getTokenRange(
               result.SourceManager->getSpellingLoc(sub_expr->getLocStart()),
               result.SourceManager->getSpellingLoc(sub_expr->getLocEnd()));
-      if (!sub_expr_range.isValid())
-        return;  // TODO(rsleevi): Log an error?
+      assert(sub_expr_range.isValid() && "Invalid subexpression range!");
+
       std::string inner_text = clang::Lexer::getSourceText(
           sub_expr_range, *result.SourceManager, result.Context->getLangOpts());
-      if (inner_text.empty())
-        return;  // TODO(rsleevi): Log an error?
-
+      assert(!inner_text.empty() && "No text for subexpression!");
       if (NeedsParens(sub_expr)) {
         inner_text.insert(0, "(");
         inner_text.append(")");
       }
       inner_text.append("->get()");
-      replacements_->insert(
-          Replacement(*result.SourceManager, range, inner_text));
-      return;
+      return Replacement(*result.SourceManager, range, inner_text);
     }
   }
 
   std::string text = clang::Lexer::getSourceText(
       range, *result.SourceManager, result.Context->getLangOpts());
-  if (text.empty())
-    return;  // TODO(rsleevi): Log an error?
+  assert(!text.empty() && "No text for expression!");
 
   // Unwrap any temporaries - for example, custom iterators that return
   // scoped_refptr<T> as part of operator*. Any such iterators should also
   // be declaring a scoped_refptr<T>* operator->, per C++03 24.4.1.1 (Table 72)
   if (const clang::CXXBindTemporaryExpr* op =
-          llvm::dyn_cast<clang::CXXBindTemporaryExpr>(arg)) {
-    arg = op->getSubExpr();
+          llvm::dyn_cast<clang::CXXBindTemporaryExpr>(expr)) {
+    expr = op->getSubExpr();
   }
 
   // Handle iterators (which are operator* calls, followed by implicit
   // conversions) by rewriting *it as it->get()
   if (const clang::CXXOperatorCallExpr* op =
-          llvm::dyn_cast<clang::CXXOperatorCallExpr>(arg)) {
+          llvm::dyn_cast<clang::CXXOperatorCallExpr>(expr)) {
     if (op->getOperator() == clang::OO_Star) {
       // Note that this doesn't rewrite **it correctly, since it should be
       // rewritten using parens, e.g. (*it)->get(). However, this shouldn't
@@ -158,19 +120,53 @@ void GetRewriterCallback::run(const MatchFinder::MatchResult& result) {
       // storing pointers to a scoped_refptr in a container.
       text.erase(0, 1);
       text.append("->get()");
-      replacements_->insert(Replacement(*result.SourceManager, range, text));
-      return;
+      return Replacement(*result.SourceManager, range, text);
     }
   }
 
   // The only remaining calls should be non-dereferencing calls (eg: member
   // calls), so a simple ".get()" appending should suffice.
-  if (NeedsParens(arg)) {
+  if (NeedsParens(expr)) {
     text.insert(0, "(");
     text.append(")");
   }
   text.append(".get()");
-  replacements_->insert(Replacement(*result.SourceManager, range, text));
+  return Replacement(*result.SourceManager, range, text);
+}
+
+Replacement RewriteRawPtrToScopedRefptr(const MatchFinder::MatchResult& result,

[dcheng, 2014/08/20 07:02:30]

This is just a move of the original code, re-adding back some of the early
returns {that existed before it was turned into a function} in the form of
asserts.

+                                        clang::SourceLocation begin,
+                                        clang::SourceLocation end) {
+  clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(
+      result.SourceManager->getSpellingLoc(begin),
+      result.SourceManager->getSpellingLoc(end));
+  assert(range.isValid() && "Invalid range!");
+
+  std::string text = clang::Lexer::getSourceText(
+      range, *result.SourceManager, result.Context->getLangOpts());
+  text.erase(text.rfind('*'));
+
+  std::string replacement_text("scoped_refptr<");
+  replacement_text += text;
+  replacement_text += ">";
+
+  return Replacement(*result.SourceManager, range, replacement_text);
+}
+
+class GetRewriterCallback : public MatchFinder::MatchCallback {
+ public:
+  explicit GetRewriterCallback(Replacements* replacements)
+      : replacements_(replacements) {}
+  virtual void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  Replacements* const replacements_;
+};
+
+void GetRewriterCallback::run(const MatchFinder::MatchResult& result) {
+  const clang::Expr* arg = result.Nodes.getNodeAs<clang::Expr>("arg");
+  assert(arg && "Unexpected match! No Expr captured!");
+  replacements_->insert(RewriteImplicitToExplicitConversion(result, arg));
 }
 
 class VarRewriterCallback : public MatchFinder::MatchCallback {
@@ -186,9 +182,7 @@ class VarRewriterCallback : public MatchFinder::MatchCallback {
 void VarRewriterCallback::run(const MatchFinder::MatchResult& result) {
   const clang::DeclaratorDecl* const var_decl =
       result.Nodes.getNodeAs<clang::DeclaratorDecl>("var");
-
-  if (!var_decl)
-    return;
+  assert(var_decl && "Unexpected match! No VarDecl captured!");
 
   const clang::TypeSourceInfo* tsi = var_decl->getTypeSourceInfo();
 
@@ -221,9 +215,7 @@ class FunctionRewriterCallback : public MatchFinder::MatchCallback {
 void FunctionRewriterCallback::run(const MatchFinder::MatchResult& result) {
   const clang::FunctionDecl* const function_decl =
       result.Nodes.getNodeAs<clang::FunctionDecl>("fn");
-
-  if (!function_decl)
-    return;
+  assert(function_decl && "Unexpected match! No FunctionDecl captured!");
 
   // If matched against an implicit conversion to a DeclRefExpr, make sure the
   // referenced declaration is of class type, e.g. the tool skips trying to
@@ -242,6 +234,22 @@ void FunctionRewriterCallback::run(const MatchFinder::MatchResult& result) {
   }
 }
 
+class MacroRewriterCallback : public MatchFinder::MatchCallback {
+ public:
+  explicit MacroRewriterCallback(Replacements* replacements)
+      : replacements_(replacements) {}
+  virtual void run(const MatchFinder::MatchResult& result) override;
+
+ private:
+  Replacements* const replacements_;
+};
+
+void MacroRewriterCallback::run(const MatchFinder::MatchResult& result) {
+  const clang::Expr* const expr = result.Nodes.getNodeAs<clang::Expr>("expr");
+  assert(expr && "Unexpected match! No Expr captured!");
+  replacements_->insert(RewriteImplicitToExplicitConversion(result, expr));
+}
+
 }  // namespace
 
 static llvm::cl::extrahelp common_help(CommonOptionsParser::HelpMessage);
@@ -255,15 +263,14 @@ int main(int argc, const char* argv[]) {
   MatchFinder match_finder;
   Replacements replacements;
 
+  auto is_scoped_refptr = recordDecl(isSameOrDerivedFrom("::scoped_refptr"),
+                                     isTemplateInstantiation());
+
   // Finds all calls to conversion operator member function. This catches calls
   // to "operator T*", "operator Testable", and "operator bool" equally.
-  auto base_matcher =
-      id("call",
-         memberCallExpr(
-             thisPointerType(recordDecl(isSameOrDerivedFrom("::scoped_refptr"),
-                                        isTemplateInstantiation())),
-             callee(conversionDecl()),
-             on(id("arg", expr()))));
+  auto base_matcher = memberCallExpr(thisPointerType(is_scoped_refptr),
+                                     callee(conversionDecl()),
+                                     on(id("arg", expr())));
 
   // The heuristic for whether or not converting a temporary is 'unsafe'. An
   // unsafe conversion is one where a temporary scoped_refptr<T> is converted to
@@ -307,6 +314,16 @@ int main(int argc, const char* argv[]) {
   auto bool_conversion_matcher = hasParent(
       expr(anyOf(implicit_to_bool, expr(hasParent(implicit_to_bool)))));
 
+  auto is_logging_helper =
+      functionDecl(anyOf(hasName("CheckEQImpl"), hasName("CheckNEImpl")));
+  auto is_gtest_helper = functionDecl(
+      anyOf(methodDecl(ofClass(recordDecl(isSameOrDerivedFrom(
+                           hasName("::testing::internal::EqHelper")))),
+                       hasName("Compare")),
+            hasName("::testing::internal::CmpHelperNE")));
+  auto is_gtest_assertion_result_ctor = constructorDecl(ofClass(
+      recordDecl(isSameOrDerivedFrom(hasName("::testing::AssertionResult")))));
+
   // Find all calls to an operator overload that are 'safe'.
   //
   // All bool conversions will be handled with the Testable trick, but that
@@ -316,7 +333,13 @@ int main(int argc, const char* argv[]) {
   match_finder.addMatcher(
       memberCallExpr(
           base_matcher,
-          unless(anyOf(is_unsafe_temporary_conversion, is_unsafe_return))),
+          // Excluded since the conversion may be unsafe.
+          unless(anyOf(is_unsafe_temporary_conversion, is_unsafe_return)),
+          // Excluded since the conversion is deep inside a macro that needs to
+          // be specially handled.
+          unless(hasAncestor(decl(anyOf(is_logging_helper,
+                                        is_gtest_helper,
+                                        is_gtest_assertion_result_ctor))))),
       &get_callback);
 
   // Find temporary scoped_refptr<T>'s being unsafely assigned to a T*.
@@ -339,6 +362,49 @@ int main(int argc, const char* argv[]) {
   match_finder.addMatcher(memberCallExpr(base_matcher, is_unsafe_return),
                           &fn_callback);
 
+  // Rewrite logging / gtest expressions that result in an implicit conversion.
+  // Luckily, the matchers don't need to handle the case where one of the macro
+  // arguments is NULL, such as:
+  // CHECK_EQ(my_scoped_refptr, NULL)
+  // because it simply doesn't compile--since NULL is actually of integral type,
+  // this doesn't trigger scoped_refptr<T>'s implicit conversion. Since there is
+  // no comparison overload for scoped_refptr<T> and int, this fails to compile.
+  MacroRewriterCallback macro_callback(&replacements);
+  // CHECK_EQ/CHECK_NE helpers.
+  match_finder.addMatcher(
+      callExpr(callee(is_logging_helper),
+               argumentCountIs(3),
+               hasAnyArgument(id("expr", expr(hasType(is_scoped_refptr)))),
+               hasAnyArgument(hasType(pointerType())),
+               hasArgument(2, stringLiteral())),
+      &macro_callback);
+  // ASSERT_EQ/ASSERT_NE/EXPECT_EQ/EXPECT_EQ, which use the same underlying
+  // helper functions. Even though gtest has special handling for pointer to
+  // NULL comparisons, it doesn't trigger in this case, so no special handling
+  // is needed for the replacements.
+  match_finder.addMatcher(
+      callExpr(callee(is_gtest_helper),
+               argumentCountIs(4),
+               hasArgument(0, stringLiteral()),
+               hasArgument(1, stringLiteral()),
+               hasAnyArgument(id("expr", expr(hasType(is_scoped_refptr)))),
+               hasAnyArgument(hasType(pointerType()))),
+      &macro_callback);
+  // ASSERT_TRUE/EXPECT_TRUE helpers. Note that this matcher doesn't need to
+  // handle ASSERT_FALSE/EXPECT_FALSE, because it gets coerced to bool before
+  // being passed as an argument to AssertionResult's constructor. As a result,
+  // GetRewriterCallback handles this case properly since the conversion isn't
+  // hidden inside AssertionResult, and the generated replacement properly
+  // rewrites the macro argument.
+  // However, the tool does need to handle the _TRUE counterparts, since the
+  // conversion occurs inside the constructor in those cases.
+  match_finder.addMatcher(
+      constructExpr(
+          argumentCountIs(2),
+          hasArgument(0, id("expr", expr(hasType(is_scoped_refptr)))),
+          hasDeclaration(is_gtest_assertion_result_ctor)),
+      &macro_callback);
+
   std::unique_ptr<clang::tooling::FrontendActionFactory> factory =
       clang::tooling::newFrontendActionFactory(&match_finder);
   int result = tool.run(factory.get());