ABataev added a comment.
In D76342#2562097 <https://reviews.llvm.org/D76342#2562097>, @Meinersbur wrote:
> Who is going to commit?
I can commit it as soon as you accepted it if you don't mind of course.
================
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,
----------------
Meinersbur wrote:
> ABataev wrote:
> > Meinersbur wrote:
> > > 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.
> > I just prefer not to expose internals if they could be processed within the
> > class instance.
> Using SmallVectorImpl for this is still the dominant style in clang. For
> instance:
> * Sema::getUndefinedButUsed
> * Sema::FindHiddenVirtualMethods
> * Sema::CollectMultipleMethodsInGlobalPool
> * Sema::SelectBestMethod
> * Sema::CollectIvarsToConstructOrDestruct
> * Sema::collectUnexpandedParameterPacks
> * Sema::FindProtocolDeclaration
> * Sema::GatherArgumentsForCall
> * Sema::GatherGlobalCodeCompletions
> * Sema::getUndefinedButUsed
> * ASTContext::getOverriddenMethods
> * ASTContext::DeepCollectObjCIvars
> * ASTContext::getInjectedTemplateArgs
>
> ...
>
There is also another motivation behind this solution: to reuse the code as
much as possible and avoid code duplication (loops to process inner
loops/bodies) and focus just on loops/bodies processing.
================
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);
----------------
Meinersbur wrote:
> ABataev wrote:
> > Meinersbur wrote:
> > > 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) {
> > > ```
> > >
> > I believe it just saves us from the extra constructor call.
> It does not: https://godbolt.org/z/81ac7o
Yes, currently it is not, it was before. But even now there is a difference in
AST:
`-CompoundStmt <col:34, line:10:1>
|-DeclStmt <line:4:5, line:6:6>
| `-VarDecl <line:4:5, line:6:5> line:4:17 used Lambda '(lambda at
line:4:26)':'(lambda at line:4:26)' cinit
| `-ExprWithCleanups <col:26, line:6:5> '(lambda at line:4:26)':'(lambda
at line:4:26)'
| `-CXXConstructExpr <line:4:26, line:6:5> '(lambda at
line:4:26)':'(lambda at line:4:26)' 'void ((lambda at line:4:26) &&) noexcept'
elidable
| `-MaterializeTemporaryExpr <line:4:26, line:6:5> '(lambda at
line:4:26)' xvalue
With `&&`:
`-CompoundStmt <col:34, line:10:1>
|-DeclStmt <line:4:5, line:6:6>
| `-VarDecl <line:4:5, line:6:5> line:4:17 used Lambda '(lambda at
line:4:26) &&' cinit
| `-ExprWithCleanups <col:26, line:6:5> '(lambda at line:4:26)' xvalue
| `-MaterializeTemporaryExpr <line:4:26, line:6:5> '(lambda at
line:4:26)' xvalue extended by Var 0x562226678ad8 'Lambda' '(lambda at
line:4:26) &&'
Just the constructor call is marked as elidable and is not emitted anymore, but
it still saves some memory/time because we don't need to emit an extra AST node.
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
