lebedev.ri marked 3 inline comments as done. lebedev.ri 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. ---------------- 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. 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
