[PATCH] D58523: [OpenMP 5.0] Parsing/sema support for to and from clauses with mapper modifier

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 12 inline comments as done.
lildmh added inline comments.


================
Comment at: lib/AST/OpenMPClause.cpp:1469
 
 void OMPClausePrinter::VisitOMPFromClause(OMPFromClause *Node) {
   if (!Node->varlist_empty()) {
----------------
ABataev wrote:
> Better to split this patch into 2: one for `to` clause and another one for `from` clause
Hi Alexey,

Thanks for the quick review! The code for `to` and `from` clauses are almost identical, so I prefer to have them in the same patch. Besides I don't have commit privilege, so it causes more time to me to have multiple patches. But if you think it's crucial to split it. I can do it. What do you think?


================
Comment at: lib/Sema/SemaOpenMP.cpp:13061
         MapperIdScopeSpec.getWithLocInContext(SemaRef.Context), MapperId,
-        /*ADL=*/false, /*Overloaded=*/true, URS.begin(), URS.end());
+        /*ADL=*/true, /*Overloaded=*/true, URS.begin(), URS.end());
   }
----------------
ABataev wrote:
> Better to fix this in a separate patch.
I can do that.


================
Comment at: lib/Sema/SemaOpenMP.cpp:13356
 
-      // Try to find the associated user-defined mapper.
-      ExprResult ER = buildUserDefinedMapperRef(
-          SemaRef, DSAS->getCurScope(), *MapperIdScopeSpec, *MapperId,
-          Type.getCanonicalType(), UnresolvedMapper);
-      if (ER.isInvalid())
-        continue;
-      MVLI.UDMapperList.push_back(ER.get());
-    } else {
-      MVLI.UDMapperList.push_back(nullptr);
     }
 
----------------
ABataev wrote:
> Are you sure about this change? This might trigger some asserts.
Yes. The original code before the mapper patch is like this. By moving it out, `to` and `from` clauses will also call `buildUserDefinedMapperRef` check the mapper as well.


================
Comment at: lib/Sema/SemaOpenMP.cpp:13413
 
-  // If the identifier of user-defined mapper is not specified, it is "default".
-  if (!MapperId.getName() || MapperId.getName().isEmpty()) {
----------------
ABataev wrote:
> Again, better to do this in a separate patch
This modification is for `to` and `from` clauses, so they also have a default mapper name. I think it is appropriate to have this piece in this patch.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58523/new/

https://reviews.llvm.org/D58523