[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:2343-2344
+    // pointer
+    if (Arg.back() < OMPC_DEFAULTMAP_MODIFIER_unknown)
+      Arg.back() = OMPC_DEFAULTMAP_MODIFIER_unknown;
     KLoc.push_back(Tok.getLocation());
----------------
cchen wrote:
> ABataev wrote:
> > Is this possible at all? I think it must an assertion rather this kind of trick.
> This is definitely possible since the `getOpenMPSimpleClauseType` function in OpenMPKinds.cpp, parse defaultmap modifier and defaultmap kind in a same stringswitch statement, so for `defaultmap(scalar`, it will set the defaultmap modifier to be 0, however, the unknown is 3 (OMPC_DEFAULTMAP_MODIFIER_unknown == OMPC_DEFAULTMAP_unknown).
Ok, I see. Then store the result in the temp var and change it there rather use `Arg.back() = ...`.I still don't like this solution but the redesign is the different problem.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:659
+    DefaultMapImplicitBehavior DMIB = getDefaultDMIB(DMVC);
+    return (DMIB == DMIB_unspecified) || (DMIB == DMIB_firstprivate) ||
+           (DMIB == DMIB_default);
----------------
Why `DMIB_firstprivate` is the default behavior? This is true only for scalars and pointers. Plus, this function is used in the context that does not match its name. Better to rename it to something like `mustBeFirstprivate` or something similar.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:663
+  bool
+  isDefaultmapDefaultBehaviorAtLevel(unsigned Level,
+                                     DefaultMapVariableCategory DMVC) const {
----------------
Same here


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2824
 
+static DefaultMapVariableCategory getVariableCategoryFromDecl(ValueDecl *VD) {
+  if (VD->getType().getNonReferenceType()->isAnyPointerType())
----------------
`const ValueDecl *`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2841-2860
+    switch (DMIB) {
+    case DMIB_alloc:
+      Kind = OMPC_MAP_alloc;
+      break;
+    case DMIB_to:
+      Kind = OMPC_MAP_to;
+      break;
----------------
Why inner switch for the same variable `DMIB`?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4451
+    SmallVector<Expr *, 4> ImplicitMaps[OMPC_MAP_delete];
+    for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+      ArrayRef<Expr *> ImplicitMap =
----------------
Use preincrement instead of postincrement.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4476
     }
-    if (!ImplicitMaps.empty()) {
-      CXXScopeSpec MapperIdScopeSpec;
-      DeclarationNameInfo MapperId;
-      if (OMPClause *Implicit = ActOnOpenMPMapClause(
-              llvm::None, llvm::None, MapperIdScopeSpec, MapperId,
-              OMPC_MAP_tofrom, /*IsMapTypeImplicit=*/true, SourceLocation(),
-              SourceLocation(), ImplicitMaps, OMPVarListLocTy())) {
-        ClausesWithImplicit.emplace_back(Implicit);
-        ErrorFound |=
-            cast<OMPMapClause>(Implicit)->varlist_size() != ImplicitMaps.size();
-      } else {
-        ErrorFound = true;
+    for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+      if (!ImplicitMaps[I].empty()) {
----------------
Use range-based loop here.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4477
+    for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+      if (!ImplicitMaps[I].empty()) {
+        CXXScopeSpec MapperIdScopeSpec;
----------------
Simplify inner complexity here by using `continue;` if the condition is true.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16393-16416
+static DefaultMapImplicitBehavior
+getImplicitBehaviorFromModifier(OpenMPDefaultmapClauseModifier M) {
+  switch (M) {
+  case OMPC_DEFAULTMAP_MODIFIER_alloc:
+    return DMIB_alloc;
+  case OMPC_DEFAULTMAP_MODIFIER_to:
+    return DMIB_to;
----------------
Do we need types `DefaultMapImplicitBehavior` and `DefaultMapVariableCategory` if the map 1 to 1 to `OpenMPDefaultmapClauseModifier` and `OpenMPDefaultmapClauseKind`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69204