lebedev.ri added inline comments.
================
Comment at: include/clang/AST/StmtOpenMP.h:335
+ llvm::Optional<Stmt *> getStructuredBlockImpl() const {
+ return const_cast<Stmt *>(getInnermostCapturedStmt()->getCapturedStmt());
----------------
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..
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59214/new/
https://reviews.llvm.org/D59214
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
