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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
