[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional<Stmt *> getStructuredBlockImpl() const {
+    return const_cast<Stmt *>(getInnermostCapturedStmt()->getCapturedStmt());
----------------
lebedev.ri wrote:
> ABataev wrote:
> > lebedev.ri wrote:
> > > ABataev wrote:
> > > > No need to insert it into each class, just add:
> > > > ```
> > > > Stmt * OMPExecutableDirective::getStructuredBlock() const {
> > > >   if (!hasAssociatedStmt() || !getAssociatedStmt())
> > > >     return nullptr;
> > > >   if (auto *LD = dyn_cast<OMPLoopDirective>(this))
> > > >     return LD->getBody();
> > > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > > }
> > > > ```
> > > I absolutely can do that, you are sure that is the most future-proof state?
> > > In particular, i want to re-point-out that if it's implemented like this,
> > > in the base class, then the sub-class may(will) not even know about this function,
> > > and thus 'forget' to update it, should it not be giving the correct answer for
> > > that new specific OpenMP executable directive.
> > > 
> > > You are sure it's better to implement it in the `OMPExecutableDirective` itself?
> > Yes, I'm sure. It is the universal solution and all future classes must be compatible with it. If they are not, then they are incorrect.
> Aha! Well, ok then.
> 
> Do you also suggest that `Optional<>` is too fancy?
> Would it be better to do this instead?
> ```
> bool isStandaloneDirective() const {
>   return !hasAssociatedStmt() || !getAssociatedStmt();
> }
> 
> // Requires: !isStandaloneDirective()
> Stmt *OMPExecutableDirective::getStructuredBlock() const {
>   assert(!isStandaloneDirective() && "Standalone Executable OpenMP directives don't have structured blocks.")
>   if (auto *LD = dyn_cast<OMPLoopDirective>(this))
>     return LD->getBody();
>   return getInnermostCapturedStmt()->getCapturedStmt();
> }
> ```
> Hm, maybe that actually conveys more meaning..
Great, that doesn't work, and highlights my concerns.
`target enter data` / `target exit data` / `target update` are stand-alone directives as per the spec,
but not as per that `isStandaloneDirective()` check ^.
https://godbolt.org/z/0tE93s

Is this a bug, or intentional?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59214/new/

https://reviews.llvm.org/D59214