[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 21 07:18:59 PDT 2019


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





More information about the cfe-commits mailing list