[PATCH] D56326: [OpenMP 5.0] Parsing/sema support for "omp declare mapper" directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: include/clang/AST/DeclBase.h:181
+    /// This declaration is an OpenMP user defined mapper.
+    IDNS_OMPMapper           = 0x2000
   };
----------------
Add comma after `0x2000` to exclude this line from the next pactches


================
Comment at: include/clang/AST/DeclOpenMP.h:220
+
+  // Clauses assoicated with this mapper declaration
+  MutableArrayRef<OMPClause *> Clauses;
----------------
Use `///` style of comments in class interface


================
Comment at: include/clang/AST/DeclOpenMP.h:224
+  // Mapper variable, which is 'v' in the example above
+  Expr *MapperVar = nullptr;
+
----------------
Better to call it `MapperVarRef`


================
Comment at: include/clang/AST/DeclOpenMP.h:279
+  clauselist_const_iterator clauselist_begin() const {
+    return static_cast<ArrayRef<const OMPClause *>>(Clauses).begin();
+  }
----------------
Just `Clauses.begin()` does not work here?


================
Comment at: include/clang/AST/DeclOpenMP.h:282
+  clauselist_const_iterator clauselist_end() const {
+    return static_cast<ArrayRef<const OMPClause *>>(Clauses).end();
+  }
----------------
Same here, the conversion looks ugly.


================
Comment at: lib/AST/ASTDumper.cpp:769
+  NodeDumper.dumpName(D);
+  for (auto *C : D->clauselists()) {
+    dumpChild([=] {
----------------
`const auto *`


================
Comment at: lib/AST/ASTDumper.cpp:786
+        OS << " <implicit>";
+      for (auto *S : C->children())
+        dumpStmt(S);
----------------
`const auto *`


================
Comment at: lib/AST/DeclOpenMP.cpp:136
+                             OMPDeclareMapperDecl *PrevDeclInScope) {
+  OMPDeclareMapperDecl *D = new (C, DC) OMPDeclareMapperDecl(
+      OMPDeclareMapper, DC, L, Name, T, VarName, PrevDeclInScope);
----------------
`auto *` or just `return new ....`


================
Comment at: lib/AST/DeclOpenMP.cpp:144
+                                                               unsigned N) {
+  OMPDeclareMapperDecl *D = new (C, ID)
+      OMPDeclareMapperDecl(OMPDeclareMapper, /*DC=*/nullptr, SourceLocation(),
----------------
`auto *`


================
Comment at: lib/AST/DeclOpenMP.cpp:150
+    OMPClause **ClauseStorage =
+        (OMPClause **)C.Allocate(sizeof(OMPClause *) * N);
+    D->Clauses = MutableArrayRef<OMPClause *>(ClauseStorage, N);
----------------
1. `auto **`
2. Use `C.Allocate<OMPClause*>(N)`


================
Comment at: lib/AST/DeclOpenMP.cpp:151
+        (OMPClause **)C.Allocate(sizeof(OMPClause *) * N);
+    D->Clauses = MutableArrayRef<OMPClause *>(ClauseStorage, N);
+  }
----------------
Use `makeMutableArrayRef(ClauseStorage, N)`


================
Comment at: lib/AST/DeclOpenMP.cpp:163
+  assert(Clauses.empty() && "Number of clauses should be 0 on initialization");
+  auto NumClauses = CL.size();
+  if (NumClauses) {
----------------
Do not use `auto` here


================
Comment at: lib/AST/DeclOpenMP.cpp:165
+  if (NumClauses) {
+    OMPClause **ClauseStorage =
+        (OMPClause **)C.Allocate(sizeof(OMPClause *) * NumClauses);
----------------
See previous comments for `ClauseStorage`


================
Comment at: lib/AST/DeclPrinter.cpp:1612
+      OMPClausePrinter Printer(Out, Policy);
+      for (auto I = D->clauselist_begin(), E = D->clauselist_end(); I != E;
+           ++I) {
----------------
Use range-based loop


================
Comment at: lib/Parse/ParseOpenMP.cpp:44
+  OMPD_target_teams_distribute_parallel,
+  OMPD_mapper
 };
----------------
Add comma after the new enum


================
Comment at: lib/Parse/ParseOpenMP.cpp:571
+  }
+  if (Clauses.size() == 0) {
+    Diag(Tok, diag::err_omp_expected_clause)
----------------
`Clauses.empty()`


================
Comment at: lib/Sema/SemaOpenMP.cpp:13427
+                                            TypeResult ParsedType) {
+  assert(ParsedType.isUsable());
+
----------------
Text message


================
Comment at: lib/Sema/SemaOpenMP.cpp:13430
+  QualType MapperType = GetTypeFromParser(ParsedType.get());
+  assert(!MapperType.isNull());
+
----------------
Add the text message for the assert


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

https://reviews.llvm.org/D56326