Index: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp |
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp |
index 6567f77918d8e70960af1cadaeb32abaeafedc7f..85a9661d70f190f8cc0f4857067739b9de772cf6 100644 |
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp |
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp |
@@ -175,11 +175,13 @@ bool IsBlacklistedFunction(const clang::FunctionDecl& decl) { |
return decl.getName() == "swap"; |
} |
-bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) { |
- if (decl.isStatic()) |
+bool IsBlacklistedMethod(const std::string& name, |
dcheng
2016/09/10 04:00:55
Pass as llvm::StringRef here, so we don't need to
Łukasz Anforowicz
2016/09/12 19:52:56
Done.
This is how I started actually, but then go
|
+ const clang::CXXMethodDecl* decl) { |
+ if (decl && decl->isStatic()) |
dcheng
2016/09/10 04:00:55
It feels like we should just have two versions of
Łukasz Anforowicz
2016/09/12 19:52:56
In the latest patchset I have 1) reverted changes
|
return false; |
- clang::StringRef name = decl.getName(); |
+ if (decl) |
+ assert(decl->getName().str() == name); |
// These methods should never be renamed. |
static const char* kBlacklistMethods[] = {"trace", "traceImpl", "lock", |
@@ -191,9 +193,15 @@ bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) { |
// Iterator methods shouldn't be renamed to work with stl and range-for |
// loops. |
- std::string ret_type = decl.getReturnType().getAsString(); |
- if (ret_type.find("iterator") != std::string::npos || |
- ret_type.find("Iterator") != std::string::npos) { |
+ bool is_iterator = false; |
+ if (decl) { |
+ std::string ret_type = decl->getReturnType().getAsString(); |
+ is_iterator = ret_type.find("iterator") != std::string::npos || |
+ ret_type.find("Iterator") != std::string::npos; |
dcheng
2016/09/10 04:00:55
For this, we can just have a simple shared method
Łukasz Anforowicz
2016/09/12 19:52:56
I think this comment is not applicable to the most
|
+ } else { |
+ is_iterator = true; |
+ } |
+ if (is_iterator) { |
static const char* kIteratorBlacklist[] = {"begin", "end", "rbegin", |
"rend"}; |
for (const auto& b : kIteratorBlacklist) { |
@@ -205,8 +213,8 @@ bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) { |
// Subclasses of InspectorAgent will subclass "disable()" from both blink and |
// from gen/, which is problematic, but DevTools folks don't want to rename |
// it or split this up. So don't rename it at all. |
- if (name.equals("disable") && |
- IsMethodOverrideOf(decl, "blink::InspectorAgent")) |
+ if (name == "disable" && |
+ (!decl || IsMethodOverrideOf(*decl, "blink::InspectorAgent"))) |
return true; |
return false; |
@@ -217,7 +225,7 @@ AST_MATCHER(clang::FunctionDecl, isBlacklistedFunction) { |
} |
AST_MATCHER(clang::CXXMethodDecl, isBlacklistedMethod) { |
- return IsBlacklistedMethod(Node); |
+ return IsBlacklistedMethod(Node.getName().str(), &Node); |
} |
// Helper to convert from a camelCaseName to camel_case_name. It uses some |
@@ -464,7 +472,6 @@ struct TargetNodeTraits<clang::NamedDecl> { |
return decl.getLocation(); |
} |
static const char* GetName() { return "decl"; } |
- static const char* GetType() { return "NamedDecl"; } |
dcheng
2016/09/10 04:00:55
While unused, these were helpful for debugging in
Łukasz Anforowicz
2016/09/12 19:52:56
Done.
|
}; |
template <> |
@@ -473,7 +480,6 @@ struct TargetNodeTraits<clang::MemberExpr> { |
return expr.getMemberLoc(); |
} |
static const char* GetName() { return "expr"; } |
- static const char* GetType() { return "MemberExpr"; } |
}; |
template <> |
@@ -482,7 +488,6 @@ struct TargetNodeTraits<clang::DeclRefExpr> { |
return expr.getLocation(); |
} |
static const char* GetName() { return "expr"; } |
- static const char* GetType() { return "DeclRefExpr"; } |
}; |
template <> |
@@ -492,7 +497,6 @@ struct TargetNodeTraits<clang::CXXCtorInitializer> { |
return init.getSourceLocation(); |
} |
static const char* GetName() { return "initializer"; } |
- static const char* GetType() { return "CXXCtorInitializer"; } |
}; |
template <> |
@@ -501,7 +505,6 @@ struct TargetNodeTraits<clang::UnresolvedLookupExpr> { |
return expr.getNameLoc(); |
} |
static const char* GetName() { return "expr"; } |
- static const char* GetType() { return "UnresolvedLookupExpr"; } |
}; |
template <> |
@@ -510,17 +513,64 @@ struct TargetNodeTraits<clang::UnresolvedMemberExpr> { |
return expr.getMemberLoc(); |
} |
static const char* GetName() { return "expr"; } |
- static const char* GetType() { return "UnresolvedMemberExpr"; } |
}; |
-template <typename DeclNode, typename TargetNode> |
+template <> |
+struct TargetNodeTraits<clang::UnresolvedUsingValueDecl> { |
+ static clang::SourceLocation GetLoc( |
+ const clang::UnresolvedUsingValueDecl& decl) { |
+ return decl.getNameInfo().getLoc(); |
+ } |
+ static const char* GetName() { return "decl"; } |
+}; |
+ |
+template <typename TargetNode> |
class RewriterBase : public MatchFinder::MatchCallback { |
public: |
explicit RewriterBase(std::set<Replacement>* replacements) |
: replacements_(replacements) {} |
+ const TargetNode& GetTargetNode(const MatchFinder::MatchResult& result) { |
+ const TargetNode* target_node = result.Nodes.getNodeAs<TargetNode>( |
+ TargetNodeTraits<TargetNode>::GetName()); |
+ assert(target_node); |
+ return *target_node; |
+ } |
+ |
+ void AddReplacement(const MatchFinder::MatchResult& result, |
+ llvm::StringRef old_name, |
+ std::string new_name) { |
+ if (old_name == new_name) |
+ return; |
+ |
+ clang::SourceLocation loc = |
+ TargetNodeTraits<TargetNode>::GetLoc(GetTargetNode(result)); |
+ clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc); |
+ replacements_->emplace(*result.SourceManager, range, new_name); |
+ replacement_names_.emplace(old_name.str(), std::move(new_name)); |
+ } |
+ |
+ const std::unordered_map<std::string, std::string>& replacement_names() |
+ const { |
+ return replacement_names_; |
+ } |
+ |
+ private: |
+ std::set<Replacement>* const replacements_; |
+ std::unordered_map<std::string, std::string> replacement_names_; |
+}; |
+ |
+template <typename DeclNode, typename TargetNode> |
+class DeclRewriterBase : public RewriterBase<TargetNode> { |
+ public: |
+ using Base = RewriterBase<TargetNode>; |
+ |
+ explicit DeclRewriterBase(std::set<Replacement>* replacements) |
+ : Base(replacements) {} |
+ |
void run(const MatchFinder::MatchResult& result) override { |
const DeclNode* decl = result.Nodes.getNodeAs<DeclNode>("decl"); |
+ assert(decl); |
// If false, there's no name to be renamed. |
if (!decl->getIdentifier()) |
return; |
@@ -542,55 +592,129 @@ class RewriterBase : public MatchFinder::MatchCallback { |
if (!GetNameForDecl(*decl, *context, new_name)) |
return; // If false, the name was not suitable for renaming. |
llvm::StringRef old_name = decl->getName(); |
- if (old_name == new_name) |
- return; |
- clang::SourceLocation loc = TargetNodeTraits<TargetNode>::GetLoc( |
- *result.Nodes.getNodeAs<TargetNode>( |
- TargetNodeTraits<TargetNode>::GetName())); |
- clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc); |
- replacements_->emplace(*result.SourceManager, range, new_name); |
- replacement_names_.emplace(old_name.str(), std::move(new_name)); |
+ Base::AddReplacement(result, old_name, std::move(new_name)); |
} |
+}; |
dcheng
2016/09/10 04:00:55
Nit: let's move the DeclRewriterBase using decls u
Łukasz Anforowicz
2016/09/12 19:52:56
Done.
|
- const std::unordered_map<std::string, std::string>& replacement_names() |
- const { |
- return replacement_names_; |
+clang::DeclarationName GetUnresolvedName( |
+ const clang::UnresolvedMemberExpr& expr) { |
+ return expr.getMemberName(); |
+} |
+ |
+clang::DeclarationName GetUnresolvedName( |
+ const clang::UnresolvedUsingValueDecl& decl) { |
+ return decl.getDeclName(); |
+} |
+ |
+// Returns whether |expr| is a callee of a method or function call. |
+bool IsCallee(const clang::Expr& expr, clang::ASTContext& context) { |
dcheng
2016/09/10 04:00:55
Can this logic be encapsulated into a matcher?
Łukasz Anforowicz
2016/09/12 19:52:56
Done. Thanks for helping figure out that I need |
|
+ auto parents = context.getParents(expr); |
+ return std::all_of( |
+ parents.begin(), parents.end(), |
+ [&expr](const clang::ast_type_traits::DynTypedNode& parent) { |
+ const clang::CallExpr* call = parent.get<clang::CallExpr>(); |
+ return call && call->getCallee() == &expr; |
+ }); |
+} |
+ |
+bool IsCallee(const clang::UnresolvedUsingValueDecl& /* decl */, |
+ clang::ASTContext& /* context */) { |
+ // Heuristic - assuming that |using Base::foo| refers to a method. |
dcheng
2016/09/10 04:00:55
We can guess from the name though, right?
Łukasz Anforowicz
2016/09/12 19:52:56
Yes, we can guess from the name - we do it in Gues
|
+ return true; |
+} |
+ |
+template <typename TargetNode> |
+class UnresolvedRewriterBase : public RewriterBase<TargetNode> { |
+ public: |
+ using Base = RewriterBase<TargetNode>; |
+ |
+ explicit UnresolvedRewriterBase(std::set<Replacement>* replacements) |
+ : RewriterBase<TargetNode>(replacements) {} |
+ |
+ void run(const MatchFinder::MatchResult& result) override { |
+ const TargetNode& expr = Base::GetTargetNode(result); |
+ llvm::StringRef old_name = GetUnresolvedName(expr).getAsString(); |
+ std::string new_name; |
+ if (GuessNameForUnresolvedDependentNode(expr, *result.Context, old_name, |
+ new_name)) { |
+ Base::AddReplacement(result, old_name, std::move(new_name)); |
+ } |
} |
private: |
- std::set<Replacement>* const replacements_; |
- std::unordered_map<std::string, std::string> replacement_names_; |
+ // This method calculates a new name for nodes that depend on template |
+ // parameters (http://en.cppreference.com/w/cpp/language/dependent_name). The |
+ // renaming is based on crude heuristics, because such nodes are not bound to |
+ // a specific decl until template instantiation - at the point of rename, one |
+ // cannot tell whether the node will eventually resolve to a field / method / |
+ // constant / etc. |
+ bool GuessNameForUnresolvedDependentNode(const TargetNode& node, |
+ clang::ASTContext& context, |
+ const std::string& old_name, |
dcheng
2016/09/10 04:00:56
Pass this as a llvm::StringRef.
Łukasz Anforowicz
2016/09/12 19:52:56
Done.
|
+ std::string& new_name) { |
+ // |m_fieldName| -> |field_name_|. |
+ if (old_name.substr(0, strlen(kBlinkFieldPrefix)) == kBlinkFieldPrefix) { |
dcheng
2016/09/10 04:00:55
Then this can just use llvm::StringRef::startswith
Łukasz Anforowicz
2016/09/12 19:52:56
Done.
|
+ std::string field_name = old_name.substr(strlen(kBlinkFieldPrefix)); |
Łukasz Anforowicz
2016/09/12 19:52:56
This is the reason why in the end I've used std::s
|
+ if (field_name.find('_') == std::string::npos) { |
+ new_name = CamelCaseToUnderscoreCase(field_name) + "_"; |
+ return true; |
+ } |
+ } |
+ |
+ if ((old_name.find('_') == std::string::npos) && IsCallee(node, context) && |
+ !IsBlacklistedMethod(old_name, nullptr)) { |
+ // |T::myMethod(...)| -> |T::MyMethod(...)|. |
+ new_name = old_name; |
+ new_name[0] = clang::toUppercase(new_name[0]); |
+ return true; |
+ } |
+ |
+ // In the future we can consider more heuristics: |
+ // - "s_" and "g_" prefixes |
+ // - "ALL_CAPS" |
+ // - |T::myStaticField| -> |T::kMyStaticField| |
+ // (but have to be careful not to rename |value| in WTF/TypeTraits.h?) |
+ return false; |
+ } |
}; |
-using FieldDeclRewriter = RewriterBase<clang::FieldDecl, clang::NamedDecl>; |
-using VarDeclRewriter = RewriterBase<clang::VarDecl, clang::NamedDecl>; |
-using MemberRewriter = RewriterBase<clang::FieldDecl, clang::MemberExpr>; |
-using DeclRefRewriter = RewriterBase<clang::VarDecl, clang::DeclRefExpr>; |
-using FieldDeclRefRewriter = RewriterBase<clang::FieldDecl, clang::DeclRefExpr>; |
+using FieldDeclRewriter = DeclRewriterBase<clang::FieldDecl, clang::NamedDecl>; |
+using VarDeclRewriter = DeclRewriterBase<clang::VarDecl, clang::NamedDecl>; |
+using MemberRewriter = DeclRewriterBase<clang::FieldDecl, clang::MemberExpr>; |
+using DeclRefRewriter = DeclRewriterBase<clang::VarDecl, clang::DeclRefExpr>; |
+using FieldDeclRefRewriter = |
+ DeclRewriterBase<clang::FieldDecl, clang::DeclRefExpr>; |
using FunctionDeclRewriter = |
- RewriterBase<clang::FunctionDecl, clang::NamedDecl>; |
+ DeclRewriterBase<clang::FunctionDecl, clang::NamedDecl>; |
using FunctionRefRewriter = |
- RewriterBase<clang::FunctionDecl, clang::DeclRefExpr>; |
+ DeclRewriterBase<clang::FunctionDecl, clang::DeclRefExpr>; |
using ConstructorInitializerRewriter = |
- RewriterBase<clang::FieldDecl, clang::CXXCtorInitializer>; |
+ DeclRewriterBase<clang::FieldDecl, clang::CXXCtorInitializer>; |
-using MethodDeclRewriter = RewriterBase<clang::CXXMethodDecl, clang::NamedDecl>; |
+using MethodDeclRewriter = |
+ DeclRewriterBase<clang::CXXMethodDecl, clang::NamedDecl>; |
using MethodRefRewriter = |
- RewriterBase<clang::CXXMethodDecl, clang::DeclRefExpr>; |
+ DeclRewriterBase<clang::CXXMethodDecl, clang::DeclRefExpr>; |
using MethodMemberRewriter = |
- RewriterBase<clang::CXXMethodDecl, clang::MemberExpr>; |
+ DeclRewriterBase<clang::CXXMethodDecl, clang::MemberExpr>; |
using EnumConstantDeclRewriter = |
- RewriterBase<clang::EnumConstantDecl, clang::NamedDecl>; |
+ DeclRewriterBase<clang::EnumConstantDecl, clang::NamedDecl>; |
using EnumConstantDeclRefRewriter = |
- RewriterBase<clang::EnumConstantDecl, clang::DeclRefExpr>; |
+ DeclRewriterBase<clang::EnumConstantDecl, clang::DeclRefExpr>; |
using UnresolvedLookupRewriter = |
- RewriterBase<clang::NamedDecl, clang::UnresolvedLookupExpr>; |
+ DeclRewriterBase<clang::NamedDecl, clang::UnresolvedLookupExpr>; |
using UnresolvedMemberRewriter = |
- RewriterBase<clang::NamedDecl, clang::UnresolvedMemberExpr>; |
+ DeclRewriterBase<clang::NamedDecl, clang::UnresolvedMemberExpr>; |
+ |
+using UnresolvedDependentMemberRewriter = |
+ UnresolvedRewriterBase<clang::UnresolvedMemberExpr>; |
-using UsingDeclRewriter = RewriterBase<clang::UsingDecl, clang::NamedDecl>; |
+using UnresolvedUsingValueDeclRewriter = |
+ UnresolvedRewriterBase<clang::UnresolvedUsingValueDecl>; |
+ |
+using UsingDeclRewriter = DeclRewriterBase<clang::UsingDecl, clang::NamedDecl>; |
} // namespace |
@@ -862,7 +986,7 @@ int main(int argc, const char* argv[]) { |
match_finder.addMatcher(unresolved_lookup_matcher, |
&unresolved_lookup_rewriter); |
- // Unresolved member expressions ======== |
+ // Unresolved member expressions (for non-dependent fields / methods) ======== |
// Similar to unresolved lookup expressions, but for methods in a member |
// context, e.g. var_with_templated_type.Method(). |
auto unresolved_member_matcher = expr(id( |
@@ -876,6 +1000,36 @@ int main(int argc, const char* argv[]) { |
match_finder.addMatcher(unresolved_member_matcher, |
&unresolved_member_rewriter); |
+ // Unresolved using value decls ======== |
+ // Example: |
+ // template <typename T> |
+ // class BaseClass { |
+ // public: |
+ // unsigned long m_size; |
+ // }; |
+ // template <typename T> |
+ // class DerivedClass : protected BaseClass<T> { |
+ // private: |
+ // using Base = BaseClass<T>; |
+ // using Base::m_size; <- matched by |unresolved_using_value_decl_matcher|. |
+ // void method() { |
+ // m_size = 123; // <- |m_size| matched by |
+ // } // |unresolved_dependent_using_matcher|. |
+ // }; |
+ auto unresolved_dependent_using_matcher = |
+ expr(id("expr", unresolvedMemberExpr(allOverloadsMatch(allOf( |
+ in_blink_namespace, unresolvedUsingValueDecl()))))); |
+ UnresolvedDependentMemberRewriter unresolved_dependent_member_rewriter( |
+ &replacements); |
+ match_finder.addMatcher(unresolved_dependent_using_matcher, |
+ &unresolved_dependent_member_rewriter); |
+ auto unresolved_using_value_decl_matcher = |
+ decl(id("decl", unresolvedUsingValueDecl(in_blink_namespace))); |
+ UnresolvedUsingValueDeclRewriter unresolved_using_value_decl_rewriter( |
+ &replacements); |
+ match_finder.addMatcher(unresolved_using_value_decl_matcher, |
+ &unresolved_using_value_decl_rewriter); |
+ |
// Using declarations ======== |
// Given |
// using blink::X; |