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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: include/clang/Basic/OpenMPKinds.def:585
+// Modifiers for 'to' and 'from' clause.
+OPENMP_TO_FROM_MODIFIER_KIND(mapper)
+
----------------
Maybe, better to split it. There must be separate modifiers for `from` clause and for `to` clause just for possible future changes.


================
Comment at: lib/AST/OpenMPClause.cpp:876
 
-  OMPToClause *Clause = new (Mem) OMPToClause(Locs, Sizes);
+  OMPToClause *Clause =
+      new (Mem) OMPToClause(UDMQualifierLoc, MapperId, Locs, Sizes);
----------------
`auto *`


================
Comment at: lib/AST/OpenMPClause.cpp:924
 
-  OMPFromClause *Clause = new (Mem) OMPFromClause(Locs, Sizes);
+  OMPFromClause *Clause =
+      new (Mem) OMPFromClause(UDMQualifierLoc, MapperId, Locs, Sizes);
----------------
`auto *`


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


================
Comment at: lib/Parse/ParseOpenMP.cpp:2154
         parseMapType(*this, Data);
+      else {
+        SkipUntil(tok::colon, tok::annot_pragma_openmp_end, StopBeforeMatch);
----------------
Remove braces


================
Comment at: lib/Parse/ParseOpenMP.cpp:2174-2175
+        if (Tok.isNot(tok::colon)) {
+          if (!IsInvalidMapperModifier)
+            Diag(Tok, diag::warn_pragma_expected_colon) << "mapper(...)";
+          SkipUntil(tok::colon, tok::r_paren, tok::annot_pragma_openmp_end,
----------------
`mapper(...)` does not look good here, we never ever used this kind of construct before. Better to use something that was used already.


================
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());
   }
----------------
Better to fix this in a separate patch.


================
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);
     }
 
----------------
Are you sure about this change? This might trigger some asserts.


================
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()) {
----------------
Again, better to do this in a separate patch


Repository:
  rC Clang

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

https://reviews.llvm.org/D58523