[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2020-2034
     if (!FirstClause) {
       Diag(Tok, diag::err_omp_more_one_clause)
           << getOpenMPDirectiveName(DKind) << getOpenMPClauseName(CKind) << 0;
       ErrorFound = true;
     }
+    Clause = ParseOpenMPSingleExprWithArgClause(CKind, WrongDirective);
+    break;
----------------
I think better just to modify the condition in the original `if`:
```
if ((getLangOpts().OpenMP < 50 || CKind != OMPC_defaultmap) && !FirstClause)
```


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:118
+    DefaultmapInfo(OpenMPDefaultmapClauseModifier M,
+                   OpenMPDefaultmapClauseKind Kind, SourceLocation Loc)
+        : ImplicitBehavior(M), SLoc(Loc) {}
----------------
`Kind` is unused here, do you really need it?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2823
+static OpenMPMapClauseKind
+getMapClauseKindFromModifier(OpenMPDefaultmapClauseModifier M, bool Cond) {
+  OpenMPMapClauseKind Kind = OMPC_MAP_unknown;
----------------
Better to rename `Cond` to something like `IsAggregateOrDeclareTarget`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2847-2851
+    if (Cond)
+      Kind = OMPC_MAP_tofrom;
+    else
+      llvm_unreachable("Unexpected defaultmap implicit behavior");
+    break;
----------------
```
    if (Cond) {
      Kind = OMPC_MAP_tofrom;
      break;
    }
    llvm_unreachable("Unexpected defaultmap implicit behavior");
```


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2953-2954
+            (!(isOpenMPTargetExecutionDirective(DKind) && Res &&
+               (*Res == OMPDeclareTargetDeclAttr::MT_Link ||
+                *Res == OMPDeclareTargetDeclAttr::MT_To)))) {
+          // Only check for data-mapping attribute and is_device_ptr here
----------------
Why comparing the value of `*Res` here if just comparing against all possible values? This condition is always `true` if `Res` is true. I don't think we need to map variables with `to` mapping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69204