[PATCH] D19382: [OpenMP] Improve mappable expressions Sema.

[Alexey Bataev via cfe-commits]

ABataev added inline comments.

================
Comment at: include/clang/AST/OpenMPClause.h:2789-2797
@@ +2788,11 @@
+    // \brief Expression associated with the component.
+    Expr *AssociatedExpression;
+    // \brief Declaration associated with the declaration. If the component does
+    // not have a declaration (e.g. array subscripts or section), this is set to
+    // nullptr.
+    ValueDecl *AssociatedDeclaration;
+
+  public:
+    MappableComponent()
+        : AssociatedExpression(nullptr), AssociatedDeclaration(nullptr) {}
+    MappableComponent(Expr *AssociatedExpression,
----------------
Please, use in-class initialization of class members, like:
```
Expr *AssociatedExpression = nullptr;
```
Also, mark this constructor as `explicit`

================
Comment at: include/clang/AST/OpenMPClause.h:2859
@@ +2858,3 @@
+  /// class.
+  ArrayRef<const ValueDecl *> getUniqueDeclsRef() const {
+    return ArrayRef<const ValueDecl *>(
----------------
I'm not sure exactly, but `ArrayRef<>` adds `const` modifier automatically, so you can use just `ValueDecl*`, without `const`, as template specialization parameter

================
Comment at: include/clang/AST/OpenMPClause.h:3069-3072
@@ +3068,6 @@
+    // The declaration the iterator currently refers to.
+    ArrayRef<const ValueDecl *>::iterator DeclCur;
+
+    // The list number associated with the current declaration.
+    ArrayRef<const unsigned>::iterator NumListsCur;
+
----------------
Again, no `const`

================
Comment at: include/clang/AST/OpenMPClause.h:3092
@@ +3091,3 @@
+    explicit const_component_lists_iterator(
+        ArrayRef<const ValueDecl *> UniqueDecls,
+        ArrayRef<unsigned> DeclsListNum, ArrayRef<unsigned> CumulativeListSizes,
----------------
`const`

================
Comment at: include/clang/AST/OpenMPClause.h:3110
@@ +3109,3 @@
+    explicit const_component_lists_iterator(
+        const ValueDecl *Declaration, ArrayRef<const ValueDecl *> UniqueDecls,
+        ArrayRef<unsigned> DeclsListNum, ArrayRef<unsigned> CumulativeListSizes,
----------------
`const`

================
Comment at: include/clang/AST/OpenMPClause.h:3135
@@ +3134,3 @@
+      if (ListSizeCur == CumulativeListSizes.end()) {
+        this->I = End;
+        RemainingLists = 0u;
----------------
remove `this->`

================
Comment at: include/clang/AST/OpenMPClause.h:3150
@@ +3149,3 @@
+      // that start the list is the size of the previous list.
+      std::advance(this->I, PrevListSize);
+    }
----------------
remove `this->`

================
Comment at: include/clang/AST/OpenMPClause.h:3160
@@ +3159,3 @@
+          *DeclCur,
+          MappableExprComponentListRef(&*this->I, *ListSizeCur - PrevListSize));
+    }
----------------
remove `this->`

================
Comment at: include/clang/AST/OpenMPClause.h:3175
@@ +3174,3 @@
+      if (std::next(ListSizeCur) == ListSizeEnd) {
+        this->I = End;
+        RemainingLists = 0;
----------------
remove `this->`

================
Comment at: include/clang/AST/OpenMPClause.h:3178
@@ +3177,3 @@
+      } else {
+        std::advance(this->I, *ListSizeCur - PrevListSize);
+        PrevListSize = *ListSizeCur;
----------------
remove `this->`

================
Comment at: include/clang/AST/OpenMPClause.h:3206
@@ +3205,3 @@
+    return const_component_lists_iterator(
+        ArrayRef<const ValueDecl *>(), ArrayRef<unsigned>(),
+        ArrayRef<unsigned>(),
----------------
`const`

================
Comment at: include/clang/AST/OpenMPClause.h:3232
@@ +3231,3 @@
+  /// components.
+  typedef ArrayRef<const ValueDecl *>::iterator const_all_decls_iterator;
+  typedef llvm::iterator_range<const_all_decls_iterator> const_all_decls_range;
----------------
`const`

================
Comment at: lib/AST/OpenMPClause.cpp:546-547
@@ +545,4 @@
+  for (auto *D : Declarations) {
+    if (Cache.count(D))
+      continue;
+    ++TotalNum;
----------------
I don't see where you modifies `Cache` set, so I presume this won't work at all.

================
Comment at: lib/Sema/SemaOpenMP.cpp:345-348
@@ -348,1 +344,6 @@
+  // any issue is found.
+  template <class MappableExprComponentListCheck>
+  bool
+  checkMappableExprComponentListsForDecl(ValueDecl *VD, bool CurrentRegionOnly,
+                                         MappableExprComponentListCheck Check) {
     auto SI = Stack.rbegin();
----------------
Maybe it can be converted to:
```
bool checkMappableExprComponentListsForDecl(ValueDecl *VD, bool CurrentRegionOnly,
                                         const llvm::function_ref<bool(OMPClauseMappableExprCommon::MappableExprComponentListRef)> &Check)
```

================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1871
@@ -1867,1 +1870,3 @@
+                                  NumComponents);
+  } break;
   case OMPC_num_teams:
----------------
`break` must be inside braces


http://reviews.llvm.org/D19382