Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.
Who is going to commit?
================
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:
> > 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
...
================
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:
> > 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
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
