sfantao added a comment. Hi Alexey,
Thanks for the review. ================ Comment at: lib/Sema/SemaOpenMP.cpp:10218-10220 @@ +10217,5 @@ + Sema &SemaRef, DSAStackTy *DSAS, OpenMPClauseKind CKind, + ArrayRef<Expr *> VarList, SmallVector<Expr *, 4> &Vars, + OMPClauseMappableExprCommon::MappableExprComponentLists &ClauseComponents, + SmallVector<ValueDecl *, 16> &ClauseBaseDeclarations, + SourceLocation StartLoc, OpenMPMapClauseKind MapType = OMPC_MAP_unknown, ---------------- ABataev wrote: > 1. Use SmallVectorImpl<Expr *> instead of SmallVector<Expr *, n>. > 2. Is it possible to reduce number of arguments of this function by gathering > them into a record? Ok, I created a new container that I named `MappableVarListInfo` to keep all the required lists. I now pass that container instead of the four lists. Hope this is more or less what you had in mind. ================ Comment at: lib/Sema/SemaOpenMP.cpp:10226-10235 @@ -10196,13 +10225,12 @@ + "Unexpected clause kind with mappable expressions!"); // Keep track of the mappable components and base declarations in this clause. // Each entry in the list is going to have a list of components associated. We // record each set of the components so that we can build the clause later on. // In the end we should have the same amount of declarations and component // lists. - OMPClauseMappableExprCommon::MappableExprComponentLists ClauseComponents; - SmallVector<ValueDecl *, 16> ClauseBaseDeclarations; - ClauseComponents.reserve(VarList.size()); ClauseBaseDeclarations.reserve(VarList.size()); for (auto &RE : VarList) { + assert(RE && "Null expr in omp to/map clause"); ---------------- Just noticed these things were unanswered. This comment was prior to the map clause patch, so now all data members cases are tested in the upstreamed regression tests. ================ Comment at: lib/Sema/SemaOpenMP.cpp:10237-10241 @@ -10210,6 +10236,7 @@ + assert(RE && "Null expr in omp to/map clause"); if (isa<DependentScopeDeclRefExpr>(RE)) { // It will be analyzed later. Vars.push_back(RE); continue; } SourceLocation ELoc = RE->getExprLoc(); ---------------- ABataev wrote: > Still think that this check is not required. Correct, this is not needed. Sorry, forgot to remove that in the previous diff. ================ Comment at: lib/Sema/SemaOpenMP.cpp:10353 @@ +10352,3 @@ + // from, release, or delete. + DKind = DSAS->getCurrentDirective(); + if (DKind == OMPD_target_exit_data && ---------------- ABataev wrote: > You already get DKind few lines above, why need to update it? Right, I don't. I removed the latest diff. ================ Comment at: test/OpenMP/nesting_of_regions.cpp:136 @@ -135,1 +135,3 @@ } +#pragma omp parallel + { ---------------- ABataev wrote: > Test 'nesting_of_regions.cpp' should be updated only when adding a new > directive, not a clause. Ok, I added the nesting tests to the dependence patch http://reviews.llvm.org/D15944. I only do a few changes in this patch to use a clause because that allows me to remove the expected errors "need to/from clause" that are unrelated with nesting. These errors are already checked by the other tests. http://reviews.llvm.org/D18597 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits