[PATCH] D58074: [OpenMP 5.0] Parsing/sema support for map clause with mapper modifier

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.
Herald added a subscriber: jdoerfert.

Also, need some kind of the stubbed codegen for the mapper



================
Comment at: include/clang/AST/OpenMPClause.h:4236
+  static OMPMapClause *
+  Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation LParenLoc,
+         SourceLocation EndLoc, ArrayRef<Expr *> Vars,
----------------
The function has more than 10 parameters, it is not too good. Would be good to pack some of them into some kind of structure.


================
Comment at: include/clang/Sema/Sema.h:9347
       ArrayRef<SourceLocation> MapTypeModifiersLoc,
+      CXXScopeSpec &MapperIdScopeSpec, DeclarationNameInfo &MapperId,
       OpenMPMapClauseKind MapType, bool IsMapTypeImplicit,
----------------
Also would be good to pack some of the params into structure


================
Comment at: include/clang/Sema/Sema.h:9431
+  OMPClause *ActOnOpenMPMapClause(
+      ArrayRef<OpenMPMapModifierKind> MapTypeModifiers,
+      ArrayRef<SourceLocation> MapTypeModifiersLoc,
----------------
Same, too many params


================
Comment at: lib/Sema/SemaLookup.cpp:1074
 
+  if (NameKind == LookupOMPMapperName) {
+    // Skip out-of-scope declarations.
----------------
Why do we need special processing of the mapper here?


================
Comment at: lib/Sema/SemaLookup.cpp:1793
           continue;
+        else if (NameKind == LookupOMPMapperName) {
+          // Skip out-of-scope declarations.
----------------
Again, what's so special in mapper lookup?


================
Comment at: lib/Sema/SemaOpenMP.cpp:13186-13192
+        if (ER.isInvalid()) {
+          continue;
+        } else if (ER.isUsable()) {
+          MVLI.UDMapperList.push_back(ER.get());
+        } else {
+          MVLI.UDMapperList.push_back(nullptr);
+        }
----------------
I think this can be simplified:
```
if (ER.isInvalid())
  continue;
MVLI.UDMapperList.push_back(ER.get());
```


================
Comment at: lib/Sema/SemaOpenMP.cpp:13232-13238
+        if (ER.isInvalid()) {
+          continue;
+        } else if (ER.isUsable()) {
+          MVLI.UDMapperList.push_back(ER.get());
+        } else {
+          MVLI.UDMapperList.push_back(nullptr);
+        }
----------------
Also can be simplified.


================
Comment at: lib/Sema/SemaOpenMP.cpp:13363-13369
+      if (ER.isInvalid()) {
+        continue;
+      } else if (ER.isUsable()) {
+        MVLI.UDMapperList.push_back(ER.get());
+      } else {
+        MVLI.UDMapperList.push_back(nullptr);
+      }
----------------
Again, simplify


================
Comment at: lib/Sema/TreeTransform.h:8845
+    } else
+      UnresolvedMappers.push_back(nullptr);
+  }
----------------
Enclose into braces


================
Comment at: lib/Serialization/ASTReader.cpp:12312
+  UDMappers.reserve(NumVars);
+  for (unsigned i = 0; i < NumVars; ++i)
+    UDMappers.push_back(Record.readExpr());
----------------
`i`->`I`


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

https://reviews.llvm.org/D58074