Meinersbur added inline comments.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:448
+ /// Number of collapsed loops as specified by 'collapse' clause.
+ unsigned CollapsedNum = 0;
+
----------------
ABataev wrote:
> Meinersbur wrote:
> > Inaccurate for the tile directive.
> We can rename it to something like `NumAssociatedLoops`
Sounds good.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:651-677
+ static bool
+ doForAllLoops(Stmt *CurStmt, bool TryImperfectlyNestedLoops,
+ unsigned NumLoops,
+ llvm::function_ref<bool(unsigned, Stmt *)> Callback);
+ static bool
+ doForAllLoops(const Stmt *CurStmt, bool TryImperfectlyNestedLoops,
+ unsigned NumLoops,
----------------
ABataev wrote:
> Meinersbur wrote:
> > Please add doxygen comments.
> >
> > IMHO, using callbacks makes the callers significantly more complicated. Why
> > not fill a SmallVectorImpl<Stmt*> with the result?
> It just hides all internal representation in the class and the user/caller
> should not worry about proper implementation of the loops processing.
> Consider it as a way to encapsulate representation details.
That could also be done by returning and iterator/filling an array with the
requested data. The latter is one of the primary uses of `SmallVectorImpl`.
coroutines should make returning an iterator much simpler, eventually when we
are allowed to use C++20.
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:659
+ llvm::function_ref<bool(unsigned, const Stmt *)> Callback) {
+ auto &&NewCallback = [Callback](unsigned Cnt, Stmt *CurStmt) {
+ return Callback(Cnt, CurStmt);
----------------
ABataev wrote:
> Meinersbur wrote:
> > I do not see why making this a forwarding reference.
> There is a type mismatch in callback types, `Stmt *` and `const Stmt *`
I understand why the additional lambda is needed, but wondered about why the
declaration is not
```
auto NewCallback = [Callback](unsigned Cnt, Stmt *CurStmt) {
```
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:680-709
+ return T->getStmtClass() == OMPSimdDirectiveClass ||
+ T->getStmtClass() == OMPForDirectiveClass ||
+ T->getStmtClass() == OMPTileDirectiveClass ||
+ T->getStmtClass() == OMPForSimdDirectiveClass ||
+ T->getStmtClass() == OMPParallelForDirectiveClass ||
+ T->getStmtClass() == OMPParallelForSimdDirectiveClass ||
+ T->getStmtClass() == OMPTaskLoopDirectiveClass ||
----------------
ABataev wrote:
> Meinersbur wrote:
> > Use `isOpenMPLoopDirective()` instead?
> I'll check if it can be used instead, though I just want to be consistent
> with what we have for other statements/expressions/directives.
It would reduce the maintenance cost of not forgetting to update this list as
well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76342/new/
https://reviews.llvm.org/D76342
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
