[PATCH] D14134: [OpenMP] Parsing and sema support for map clause

[Alexey Bataev via cfe-commits]

ABataev added inline comments.

================
Comment at: include/clang/AST/OpenMPClause.h:2628
@@ +2627,3 @@
+  friend class OMPClauseWriter;
+  friend class Sema;
+
----------------
I don't like the idea of friend Sema. If Sema needs some interfaces, they must be public.

================
Comment at: include/clang/Basic/OpenMPKinds.h:83
@@ +82,3 @@
+enum OpenMPMapClauseKind {
+  OMPC_MAP_unknown = 0,
+#define OPENMP_MAP_KIND(Name) \
----------------
This must be the last element in enum.

================
Comment at: include/clang/Basic/OpenMPKinds.h:87
@@ +86,3 @@
+#include "clang/Basic/OpenMPKinds.def"
+  NUM_OPENMP_MAP_KIND
+};
----------------
Remove this one, it is not required

================
Comment at: include/clang/Sema/Sema.h:8013-8014
@@ -8013,1 +8012,4 @@
+      OpenMPLinearClauseKind LinKind, OpenMPMapClauseKind MapTypeModifier,
+      OpenMPMapClauseKind MapType, SourceLocation DepLinLoc,
+      SourceLocation MapLoc);
   /// \brief Called on well-formed 'private' clause.
----------------
I think DepLinLoc and MapLoc can joined into one argument, like DepLinMapLoc

================
Comment at: lib/AST/StmtPrinter.cpp:853-854
@@ +852,4 @@
+    }
+    OS << getOpenMPSimpleClauseTypeName(OMPC_map, Node->getMapType());
+    OS << ':';
+
----------------
I think MapType can be optional, just like MapTypeModifier

================
Comment at: lib/AST/StmtPrinter.cpp:856-858
@@ +855,5 @@
+
+    for (OMPMapClause::varlist_iterator I = Node->varlist_begin(),
+                                        E = Node->varlist_end();
+         I != E; ++I) {
+      OS << (I == Node->varlist_begin() ? ' ' : ',')
----------------
Better to use ranged for, if possible

================
Comment at: lib/Parse/ParseOpenMP.cpp:780-781
@@ -772,2 +779,4 @@
+  bool UnexpectedId = false;
   SourceLocation DepLinLoc;
+  SourceLocation MapLoc;
 
----------------
These two can be joined

================
Comment at: lib/Sema/SemaOpenMP.cpp:7482-7484
@@ +7481,5 @@
+  bool IsCorrect = true;
+  for (DeclContext::decl_iterator I = DC->noload_decls_begin(),
+                                  E = DC->noload_decls_end();
+       I != E; ++I) {
+    if (*I) {
----------------
Use ranged loop and do not use noload version.

================
Comment at: lib/Sema/SemaOpenMP.cpp:7503-7505
@@ +7502,5 @@
+  }
+  for (CXXRecordDecl::base_class_iterator I = RD->bases_begin(),
+                                          E = RD->bases_end();
+       I != E; ++I) {
+    if (!IsCXXRecordForMappable(SemaRef, I->getLocStart(), Stack,
----------------
Ranged loop

================
Comment at: lib/Sema/SemaOpenMP.cpp:7507-7509
@@ +7506,5 @@
+    if (!IsCXXRecordForMappable(SemaRef, I->getLocStart(), Stack,
+                                I->getType()->getAsCXXRecordDecl())) {
+      IsCorrect = false;
+    }
+  }
----------------
Braces are not needed

================
Comment at: lib/Sema/SemaOpenMP.cpp:7522-7524
@@ +7521,5 @@
+    if (!RD->isInvalidDecl() &&
+        !IsCXXRecordForMappable(SemaRef, SL, Stack, RD)) {
+      return false;
+    }
+  }
----------------
Extra braces

================
Comment at: lib/Sema/SemaOpenMP.cpp:7636-7638
@@ +7635,5 @@
+    if (!CheckTypeMappable(VE->getExprLoc(), VE->getSourceRange(), *this,
+                           DSAStack, Type)) {
+      continue;
+    }
+
----------------
Extra braces

================
Comment at: test/OpenMP/target_map_messages.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -verify -fopenmp -ferror-limit 100 %s
+
----------------
Tests for templates, serialization/deserialization, AST printing, region nesting


http://reviews.llvm.org/D14134