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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
