[PATCH] D15944: [OpenMP] Parsing and sema support for target update directive

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Kelvin,

Thanks for working on this! I have some minor comments inlined.


================
Comment at: lib/Sema/SemaOpenMP.cpp:9392
@@ +9391,3 @@
+              SemaRef.Diag(ELoc, diag::err_omp_map_shared_storage) << ERange;
+            else
+              SemaRef.Diag(ELoc, 
----------------
I'd add an assertion here to make sure CKind is To/Form.

================
Comment at: lib/Sema/SemaOpenMP.cpp:9452
@@ +9451,3 @@
+          else
+            SemaRef.Diag(ELoc, 
+                         diag::err_omp_once_referenced_in_target_update)
----------------
Same thing.

================
Comment at: lib/Sema/SemaOpenMP.cpp:10045
@@ +10044,3 @@
+                                     SourceLocation EndLoc) {
+  SmallVector<Expr *, 4> Vars;
+  for (auto &RE : VarList) {
----------------
Given that these checks are the same, I think it would be better to outline the map clause implementation to some static function and pass the clause kind along. Only a few map-specific things would have to be guarded with the kind.

================
Comment at: lib/Sema/SemaOpenMP.cpp:10108
@@ +10107,3 @@
+    // environment, because the restrictions are different.
+    if (CheckMapConflicts(*this, DSAStack, D, SimpleExpr,
+                          /*CurrentRegionOnly=*/true, OMPC_to))
----------------
You should update the comment. The conflicts are not with the map clause but with to/from clauses of the same constructs. You can still say, that the concerns here are the same as in map clauses so you can reuse the same checks.

================
Comment at: lib/Sema/SemaOpenMP.cpp:10202
@@ +10201,3 @@
+    // environment, because the restrictions are different.
+    if (CheckMapConflicts(*this, DSAStack, D, SimpleExpr,
+                          /*CurrentRegionOnly=*/true, OMPC_from))
----------------
Same thing here.


http://reviews.llvm.org/D15944