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

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Alexey,

Thanks for the review!


================
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,
----------------
ABataev wrote:
> Please, use in-class initialization of class members, like:
> ```
> Expr *AssociatedExpression = nullptr;
> ```
> Also, mark this constructor as `explicit`
Done!

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

================
Comment at: include/clang/AST/OpenMPClause.h:3135
@@ +3134,3 @@
+      if (ListSizeCur == CumulativeListSizes.end()) {
+        this->I = End;
+        RemainingLists = 0u;
----------------
ABataev wrote:
> remove `this->`
We have to use `this` because `I` is member of a template parent class and cannot be resolved by the template class definition. It can only be resolved by accessing the specific template instance pointed to by `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);
+    }
----------------
ABataev wrote:
> remove `this->`
Have to keep this for the reason I mentioned above.

================
Comment at: include/clang/AST/OpenMPClause.h:3160
@@ +3159,3 @@
+          *DeclCur,
+          MappableExprComponentListRef(&*this->I, *ListSizeCur - PrevListSize));
+    }
----------------
ABataev wrote:
> remove `this->`
Have to keep this for the reason I mentioned above.

================
Comment at: include/clang/AST/OpenMPClause.h:3175
@@ +3174,3 @@
+      if (std::next(ListSizeCur) == ListSizeEnd) {
+        this->I = End;
+        RemainingLists = 0;
----------------
ABataev wrote:
> remove `this->`
Have to keep this for the reason I mentioned above.

================
Comment at: include/clang/AST/OpenMPClause.h:3178
@@ +3177,3 @@
+      } else {
+        std::advance(this->I, *ListSizeCur - PrevListSize);
+        PrevListSize = *ListSizeCur;
----------------
ABataev wrote:
> remove `this->`
Have to keep this for the reason I mentioned above.

================
Comment at: lib/AST/OpenMPClause.cpp:546-547
@@ +545,4 @@
+  for (auto *D : Declarations) {
+    if (Cache.count(D))
+      continue;
+    ++TotalNum;
----------------
ABataev wrote:
> I don't see where you modifies `Cache` set, so I presume this won't work at all.
Thanks for catching that! You are right. I fixed that. This was not causing bad behavior, other than allocating more memory than what is actually required in the trailing objects.





================
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();
----------------
ABataev wrote:
> Maybe it can be converted to:
> ```
> bool checkMappableExprComponentListsForDecl(ValueDecl *VD, bool CurrentRegionOnly,
>                                          const llvm::function_ref<bool(OMPClauseMappableExprCommon::MappableExprComponentListRef)> &Check)
> ```
Done!


http://reviews.llvm.org/D19382