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..b2cd31f87c363883d4d3aa57a22fe35297a30743 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!"); |
// 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, |
+ 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,16 @@ 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 occurs inside a helper function that |
+ // the macro wraps. Letting this callback handle the rewrite would |
+ // result in an incorrect replacement that changes the helper function |
+ // itself. Instead, the right replacement is to rewrite the macro's |
+ // arguments. |
+ 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 +365,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())), |
+ ¯o_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()))), |
+ ¯o_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)), |
+ ¯o_callback); |
+ |
std::unique_ptr<clang::tooling::FrontendActionFactory> factory = |
clang::tooling::newFrontendActionFactory(&match_finder); |
int result = tool.run(factory.get()); |