================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
