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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.


================
Comment at: lib/AST/StmtOpenMP.cpp:40
+  if (auto *LD = dyn_cast<OMPLoopDirective>(this))
+    return LD->getBody();
+  return getInnermostCapturedStmt()->getCapturedStmt();
----------------
lebedev.ri wrote:
> riccibruno wrote:
> > lebedev.ri wrote:
> > > @riccibruno
> > > `getBody()` exists in `const`-only variant
> > > `getInnermostCapturedStmt()` does exist in both the `const` and non-`const` variants,
> > > but the `const` variant does `const_cast` and defers to non-`const` variant.
> > > `getCapturedStmt()` is good though.
> > > 
> > > So at best i can add
> > > ```
> > > Stmt *getStructuredBlock() {
> > >   return const_cast<Stmt *>(const_cast<OMPExecutableDirective *>(this)->getStructuredBlock());
> > > }
> > > ```
> > > I'm not sure it's worth it?
> > Or perhaps something like the following to avoid one `const_cast`:
> > 
> > ```
> > Stmt *getStructuredBlock() {
> >   const OMPExecutableDirective *ConstThis = this;
> >   return const_cast<Stmt *>(ConstThis->getStructuredBlock());
> > }
> > ```
> > 
> > My point is that in itself this is a minor thing, but not defining consistently both versions (when appropriate) inevitably leads to annoying errors. For example suppose that you only define the const version of `getStructuredBlock()`, and then there is some random function `bar` with the signature `void bar(Stmt *S)`.
> > 
> > You then try:
> > ```
> > // Intentionally not `const Stmt *` since we will set some random flag in S.
> > void bar(Stmt *S);
> > 
> > // Non-const since we are modifying it.
> > OMPExecutableDirective *OED = something;
> > bar(OED->getStructuredBlock())
> > ```
> > 
> > which fails because `OED->getStructuredBlock()` has type `const Stmt*`, even though every other pointee type is non-const qualified.
> > 
> > So unless it is intentional to forbid modifying the statement returned by `getStructuredBlock()`, defining both versions of `getStructuredBlock()` avoid this problem. Yes it is annoying to have to define the two versions but as far as I know this is inherent to C++ unfortunately.
> Hmm, okay, will add during next update / before landing, whichever happens next.
Sounds good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214