[PATCH] First revision of the dynamic ASTMatcher library

Manuel Klimek klimek at google.com
Wed Apr 24 02:07:08 PDT 2013



================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:54
@@ +53,3 @@
+
+  static VariantValue parseMatcher(llvm::StringRef MatcherCode,
+                                   TokenProcessor *Processor);
----------------
Samuel Benzaquen wrote:
> Manuel Klimek wrote:
> > I'm wondering whether we want to call the return value differently to make sure it's usually a matcher? (I assume it's called value because it can also be just a literal value type).
> The result could also be an error. Having the error in the VariantValue makes many interfaces simpler.
> In particular, it allows this method and all the factory methods to return errors.
> 
> Also, some of the test code actually returns strings here (the processor does it) for simplicity.
Yea, I think "the result can also be an error" is what I'm unhappy about, at least with the current naming.
If VariantValue was simpler to understand I'd probably be fine with it, but it seems a tad to clever for my taste. As a user of this interface, what I really want is a matcher, and I'd like to hide the fact that somewhere in the implementation VariantValue is needed, because it can currently be either a matcher or a literal value.

================
Comment at: include/clang/ASTMatchers/Dynamic/Registry.h:31-32
@@ +30,4 @@
+  // matcher by name.
+  // It will return VariantError if the matcher is not found, or if the
+  // number of arguments or argument types do not match the signature.
+  static VariantValue constructMatcher(const std::string &MatcherName,
----------------
Samuel Benzaquen wrote:
> Manuel Klimek wrote:
> > Can we instead use Diagnostics for error reporting?
> I don't know. It looks like Diagnostics works with SourceManager/SourceLocation.
> I could make the error type have more diagnostic info, like line/col numbers.
Generally, in clang, when we parse stuff using the SourceManager/SourceLocation/Diagnostics is a good idea :) Every time I thought it was too much effort, in the end I regretted it...

================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:41
@@ +40,3 @@
+
+  class TokenProcessor {
+  public:
----------------
Samuel Benzaquen wrote:
> Samuel Benzaquen wrote:
> > Dmitri Gribenko wrote:
> > > Manuel Klimek wrote:
> > > > Since this is an interface for the Parser, I think this requires a lot more documentation to explain
> > > > a) why it is an interface (instead of just built-in)
> > > > b) why processValueToken has a default implementation
> > > > 
> > > Documentation comment?
> > Done.
> Already added some comments. Let me know if you want something extra.
Cool, thanks, that helps a lot... Why not have a default implementation that does what RealProcessor does, though? Otherwise, I think the interface here is asymmetric - it should either be fully abstract, or have decent implementations for both.


http://llvm-reviews.chandlerc.com/D714



More information about the cfe-commits mailing list