================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:131
@@ -133,1 +130,3 @@
+ std::vector<std::pair<const internal::DynTypedMatcher*, MatchCallback*> >
+ Triggers;
----------------
Michael Diamond wrote:
> I would prefer a clearer name, even if it is on the wordy side. Trigger
> doesn't really fit. Some possibilities:
> MatcherMatchActionPair
> MatcherMatchCallbackPair
> MatcherAndActionPair
Maybe: CallbackForMatcher?
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:86
@@ -71,1 +85,3 @@
+ /// pointers and thus need to be stored by value.
+ llvm::AlignedCharArrayUnion<Decl*, Stmt*, QualType> Storage;
};
----------------
Michael Diamond wrote:
> I think it would be better to use boost::any here. Then you don't have to
> explicitly specify all the types. You will still be able to store the
> QualType by value, but you won't have to worry about it in the declaration of
> Storage.
LLVM and Boost don't mix ;-).... Boost is quite massive and would create a
significant burden on clang. The specialized abstractions provided by LLVM are
quite sufficient for most use cases.
The best data structure would probably by a union, however until we can use
C++11, unions don't support members with non-trivial constructors. That is why
we use this helper structure for the time being.
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:92
@@ -77,1 +91,3 @@
+ if (Tag == NT_Decl)
+ return dyn_cast<T>(*reinterpret_cast<Decl*const*>(Storage));
return NULL;
----------------
Michael Diamond wrote:
> Why the change to reinterpret_cast? It seems a fair bit messier. This would
> also be cleaner using boost::any.
As said above, no boost in LLVM. However, I agree that we could probably
continue passing in a void* here, right? Even for the QualType we are passing a
pointer to the storage inside DynTypedNode, which should work.
http://llvm-reviews.chandlerc.com/D33
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
