aaron.ballman added inline comments.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+ // Note that we have recieved a *matcher* for the clause, not the
+ // OpenMPClauseKind. We now need to extract the 'return' type of said
matcher,
+ // and convert it to the OpenMPClauseKind, so we can finally use that.
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > lebedev.ri wrote:
> > > > > > gribozavr wrote:
> > > > > > > lebedev.ri wrote:
> > > > > > > > gribozavr wrote:
> > > > > > > > > Why not make `isAllowedToContainClause` take an
> > > > > > > > > `OpenMPClauseKind` enum value?
> > > > > > > > >
> > > > > > > > > I don't see right now advantages for taking a matcher. (For
> > > > > > > > > example, it can't be a more complex matcher with inner
> > > > > > > > > matchers, it can't be a disjunction of matchers etc.)
> > > > > > > > I don't feel like it, it's uglier.
> > > > > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > > > > Also, how will passing some random enum work with e.g.
> > > > > > > > clang-query?
> > > > > > > >
> > > > > > > There are dozens of clauses in `OpenMPClauseKind`. We would have
> > > > > > > to replicate them all as matchers to provide a useful API.
> > > > > > >
> > > > > > > > Also, how will passing some random enum work with e.g.
> > > > > > > > clang-query?
> > > > > > >
> > > > > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > > > > True. Also, but there's dosens of Stmt types, and there is no
> > > > > > overload that takes `StmtClass` enum.
> > > > > For Stmts, we do have dozens of individual matchers for them.
> > > > >
> > > > > The point of your work is to add ASTMatchers for OpenMP, right?
> > > > > However, if there are no matchers for a reasonable amount of AST
> > > > > surface, it is as good as if the matchers are not there, because
> > > > > prospective users won't be able to use them.
> > > > >
> > > > > I don't particularly care how exactly this is achieved, through
> > > > > individual matchers or through a matcher that takes an enum.
> > > > > However, I want to make sure that if you're going through all this
> > > > > trouble to add matchers, the resulting API should cover a good amount
> > > > > of AST.
> > > > >
> > > > > The reason why I suggested to pass the enum to the matcher is simply
> > > > > because it is less code duplication, less work, and more reliable
> > > > > code (since there will be only one matcher to review, test, and
> > > > > maintain, instead of combinations of matchers).
> > > > >
> > > > > Another reason to not use an inner matcher here is the peculiar
> > > > > semantics of this function -- it does not evaluate the matcher, and
> > > > > it does not accept a matcher expression of any shape.
> > > > > The point of your work is to add ASTMatchers for OpenMP, right?
> > > >
> > > > Absolutely not.
> > > > D57113 + D59466 is the one and only point, to address the bugs i have
> > > > personally encountered.
> > > > The whole reason why i have started off with NOT adding these matchers
> > > > to the `ASTMatchers.h`,
> > > > but keeping them at least initially internal to the checks was to avoid
> > > > all this bikeshedding.
> > > However, I do care about the AST matchers being usable by other clients.
> > >
> > > I also care about the API following existing patterns:
> > >
> > > > Another reason to not use an inner matcher here is the peculiar
> > > > semantics of this function -- it does not evaluate the matcher, and it
> > > > does not accept a matcher expression of any shape.
> > >
> > >
> > >> Also, how will passing some random enum work with e.g. clang-query?
> > > See llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h.
> >
> > That doesn't mean it works super well, though. String literals more easily
> > contain silent typos, don't have autocomplete support, etc. I can
> > definitely sympathize with not wanting to use an enum here.
> >
> > However, I see that there are 50+ enumerations in this list -- that seems
> > like too many matchers to want to expose. I think an enum will be the
> > better, more maintainable option. The current approach won't scale well.
> Okay, but apparently clang-query will needs to be fixed too:
> ```
> clang-query> match
> stmt(ompExecutableDirective(isAllowedToContainClause(OMPC_default)))
> 1:1: Error parsing argument 1 for matcher stmt.
> 1:6: Error parsing argument 1 for matcher ompExecutableDirective.
> 1:29: Error parsing argument 1 for matcher isAllowedToContainClause.
> 1:58: Error parsing matcher. Found token <_> while looking for '('.
>
> ```
> https://bugs.llvm.org/show_bug.cgi?id=41176
clang-query requires enumerations to be quoted string literals. If you switch
to that in your test, does it work for you? I was spotting some odd behavior
with a different matcher (the attribute one, which documents the quoting
requirement).
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:6473-6475
+/// FIXME: this matcher does not work with clang-query because clang-query
+/// fails to handle ``OMPC_default`` as a param.
+/// https://bugs.llvm.org/show_bug.cgi?id=41176
----------------
See the matcher for `hasAttr()` -- we should use similar exposition here.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57112/new/
https://reviews.llvm.org/D57112
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
