[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Alexey Bataev via Phabricator via cfe-commits]

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