Meinersbur added a comment. In D83261#2162561 <https://reviews.llvm.org/D83261#2162561>, @ABataev wrote:
> 1. OMPChildren class uses standard TrailingObjects harness instead of manual > calculation. I was going to argue that OMPExecutableDirective could derive from TrailingObjects as well, but it requires a template parameter for its final subclass. Good point! > 2. Child Stmt* based nodes are not related to the AsssociatedStmt* anymore > and can exist independently. It moved into the OMPChildren class. Not sure whether it is better. >> OMPChildren also handles clauses for OMPExecutableDirectives but not for >> declarative directives. Should handling of of clauses also be extracted into >> into its own class? That would make (de-)serialization easier for those as >> well. > > This class is only for executable directives. Nearly all directives have clauses. At the moment they all implement their own clause list. I don't see why clause management should be centralized for executable statements only. >> There is no effect on D76342 <https://reviews.llvm.org/D76342> (except a >> requiring a rebase), since the OMPTileDirective has children and thus does >> not profit from the functionality of `OMPChildren` becoming optional. Since >> I don't see a relation to D76342 <https://reviews.llvm.org/D76342>, more , I >> am indifferent to whether this should be merged. > > There should be an additional patch, which, I hope, should simplify things > for loop-based directives. OK. What does this patch do? Are you going to upload it as well? >> Trailing objects is a technique to ensure that all substmts are consecutive >> in memory (so `StmtIterator` can iterator over them). For >> OMPExeuctableDirectives, only the associated statement is returned by the >> `StmtIterator`, i.e. all the children could be made regular class members >> without the complication of computing the offset. I'd prefer that change >> over OMPChildren. > > There are also used_children, which are used by the clang analyzer for, at > least, use of uninitialized variables diagnostics. OK, I see. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83261/new/ https://reviews.llvm.org/D83261 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
