gribozavr 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: > 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57112/new/ https://reviews.llvm.org/D57112 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
