[PATCH] D18597: [OpenMP] Parsing and sema support for the to clause

[Samuel Antao via cfe-commits]

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