ABataev added a comment. In D83261#2162904 <https://reviews.llvm.org/D83261#2162904>, @Meinersbur wrote:
> 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. What I mean, that previously child nodes could not exist without AssociatedStmt, i.e. you have to have AssociatedStmt to have other children. It is solved (though, of course, can be implemented in a different way with the current design). > > >>> 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. Sure, we can make `OMPChildren` common for declarative and executable directives. Do you want me to do it? > > >>> 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? At first, need to deal with this one, at least come to an agreement with the design. > > >>> 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
