================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:132-133
@@ -127,7 +131,4 @@
private:
- /// \brief The MatchCallback*'s will be called every time the
- /// UntypedBaseMatcher matches on the AST.
- std::vector< std::pair<
- const internal::UntypedBaseMatcher*,
- MatchCallback*> > Triggers;
+ /// \brief The triggers executed against the AST.
+ std::vector<Trigger> Triggers;
----------------
Sean Silva wrote:
> Since "trigger" is just a plain pair, it's not really clear at all what
> "executed against the AST means" for a trigger without some supporting
> documentation. Also as I mentioned above, putting the definition of `Trigger`
> nearby would help too.
>
> (in general, a more self-documenting name than "trigger" would be good
> ("MatcherWithAction"?)).
Yea, I didn't really like the name either, and the more I think about it, the
worse "Trigger" sounds.
How about MatchAction?
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:92-132
@@ -71,23 +91,43 @@
};
template<typename T> struct DynTypedNode::BaseConverter<T,
- typename llvm::enable_if<llvm::is_base_of<
- Decl, typename llvm::remove_pointer<T>::type > >::type > {
- static Decl *get(NodeTypeTag Tag, void *Node) {
- if (Tag == NT_Decl) return static_cast<Decl*>(Node);
+ typename llvm::enable_if<llvm::is_base_of<Decl, T> >::type> {
+ static const T *get(NodeTypeTag Tag, const char Storage[]) {
+ if (Tag == NT_Decl)
+ return dyn_cast<T>(*reinterpret_cast<Decl*const*>(Storage));
return NULL;
}
- static DynTypedNode create(const Decl *Node) {
- return DynTypedNode(NT_Decl, Node);
+ static DynTypedNode create(const Decl &Node) {
+ DynTypedNode Result;
+ Result.Tag = NT_Decl;
+ new (Result.Storage.buffer) const Decl*(&Node);
+ return Result;
}
};
template<typename T> struct DynTypedNode::BaseConverter<T,
- typename llvm::enable_if<llvm::is_base_of<
- Stmt, typename llvm::remove_pointer<T>::type > >::type > {
- static Stmt *get(NodeTypeTag Tag, void *Node) {
- if (Tag == NT_Stmt) return static_cast<Stmt*>(Node);
+ typename llvm::enable_if<llvm::is_base_of<Stmt, T> >::type> {
+ static const T *get(NodeTypeTag Tag, const char Storage[]) {
+ if (Tag == NT_Stmt)
+ return dyn_cast<T>(*reinterpret_cast<Stmt*const*>(Storage));
return NULL;
}
- static DynTypedNode create(const Stmt *Node) {
- return DynTypedNode(NT_Stmt, Node);
+ static DynTypedNode create(const Stmt &Node) {
+ DynTypedNode Result;
+ Result.Tag = NT_Stmt;
+ new (Result.Storage.buffer) const Stmt*(&Node);
+ return Result;
}
};
+template<> struct DynTypedNode::BaseConverter<QualType, void> {
+ static const QualType *get(NodeTypeTag Tag, const char Storage[]) {
+ if (Tag == NT_QualType)
+ return reinterpret_cast<const QualType*>(Storage);
+ return NULL;
+ }
+ static DynTypedNode create(const QualType &Node) {
+ DynTypedNode Result;
+ Result.Tag = NT_QualType;
+ new (Result.Storage.buffer) QualType(Node);
+ return Result;
+ }
+};
+// The only operation we allow on unsupported types is \c get.
----------------
Sean Silva wrote:
> It seems like all of these traits classes are basically the same boilerplate
> with NT_* used for the tag and the actual stored type used as the type of
> the placement new and some reinterpret casting.
>
> The only one that seems to differ between them is `get` which for Decl and
> Stmt has a dyn_cast, while QualType doesn't.
>
> It would be nice if this duplication could be reduced, for example by having
> the traits class explicitly encode the constant (e.g. in an enum), and
> implementing as must as possible generically. I not, then at least a comment
> explaining the duplication situation would be good.
The problem is that the QualType one is sufficiently different (using the
storage for storage of the actual object).
The other two are similar enough, but on the other hand I'm not sure whether
pulling out yet another abstraction is a good idea already.
================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:81-85
@@ -81,3 +80,7 @@
reset();
- traverse(Node);
+ if (const Decl *D = DynNode.get<Decl>())
+ traverse(*D);
+ else if (const Stmt *S = DynNode.get<Stmt>())
+ traverse(*S);
+ // FIXME: Add other base types after adding tests.
return Matches;
----------------
Sean Silva wrote:
> A switch on the kind, with an "unreachable" default would allow clang to warn
> on incomplete coverage of the enum during future maintenance.
I actually don't want to expose the kind enum. Perhaps I'm a little too
cautious, but I like my interfaces as small as possible.
I'm more thinking about having something built into RAV that would make sure
all types are handled, but that would make RAV depend on the DynTypedNodes. I
guess we'll see how it goes...
http://llvm-reviews.chandlerc.com/D33
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
