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