ABataev added inline comments.
================
Comment at: include/clang/AST/DeclBase.h:181
+ /// This declaration is an OpenMP user defined mapper.
+ IDNS_OMPMapper = 0x2000
};
----------------
Add comma after `0x2000` to exclude this line from the next pactches
================
Comment at: include/clang/AST/DeclOpenMP.h:220
+
+ // Clauses assoicated with this mapper declaration
+ MutableArrayRef<OMPClause *> Clauses;
----------------
Use `///` style of comments in class interface
================
Comment at: include/clang/AST/DeclOpenMP.h:224
+ // Mapper variable, which is 'v' in the example above
+ Expr *MapperVar = nullptr;
+
----------------
Better to call it `MapperVarRef`
================
Comment at: include/clang/AST/DeclOpenMP.h:279
+ clauselist_const_iterator clauselist_begin() const {
+ return static_cast<ArrayRef<const OMPClause *>>(Clauses).begin();
+ }
----------------
Just `Clauses.begin()` does not work here?
================
Comment at: include/clang/AST/DeclOpenMP.h:282
+ clauselist_const_iterator clauselist_end() const {
+ return static_cast<ArrayRef<const OMPClause *>>(Clauses).end();
+ }
----------------
Same here, the conversion looks ugly.
================
Comment at: lib/AST/ASTDumper.cpp:769
+ NodeDumper.dumpName(D);
+ for (auto *C : D->clauselists()) {
+ dumpChild([=] {
----------------
`const auto *`
================
Comment at: lib/AST/ASTDumper.cpp:786
+ OS << " <implicit>";
+ for (auto *S : C->children())
+ dumpStmt(S);
----------------
`const auto *`
================
Comment at: lib/AST/DeclOpenMP.cpp:136
+ OMPDeclareMapperDecl *PrevDeclInScope) {
+ OMPDeclareMapperDecl *D = new (C, DC) OMPDeclareMapperDecl(
+ OMPDeclareMapper, DC, L, Name, T, VarName, PrevDeclInScope);
----------------
`auto *` or just `return new ....`
================
Comment at: lib/AST/DeclOpenMP.cpp:144
+ unsigned N) {
+ OMPDeclareMapperDecl *D = new (C, ID)
+ OMPDeclareMapperDecl(OMPDeclareMapper, /*DC=*/nullptr, SourceLocation(),
----------------
`auto *`
================
Comment at: lib/AST/DeclOpenMP.cpp:150
+ OMPClause **ClauseStorage =
+ (OMPClause **)C.Allocate(sizeof(OMPClause *) * N);
+ D->Clauses = MutableArrayRef<OMPClause *>(ClauseStorage, N);
----------------
1. `auto **`
2. Use `C.Allocate<OMPClause*>(N)`
================
Comment at: lib/AST/DeclOpenMP.cpp:151
+ (OMPClause **)C.Allocate(sizeof(OMPClause *) * N);
+ D->Clauses = MutableArrayRef<OMPClause *>(ClauseStorage, N);
+ }
----------------
Use `makeMutableArrayRef(ClauseStorage, N)`
================
Comment at: lib/AST/DeclOpenMP.cpp:163
+ assert(Clauses.empty() && "Number of clauses should be 0 on initialization");
+ auto NumClauses = CL.size();
+ if (NumClauses) {
----------------
Do not use `auto` here
================
Comment at: lib/AST/DeclOpenMP.cpp:165
+ if (NumClauses) {
+ OMPClause **ClauseStorage =
+ (OMPClause **)C.Allocate(sizeof(OMPClause *) * NumClauses);
----------------
See previous comments for `ClauseStorage`
================
Comment at: lib/AST/DeclPrinter.cpp:1612
+ OMPClausePrinter Printer(Out, Policy);
+ for (auto I = D->clauselist_begin(), E = D->clauselist_end(); I != E;
+ ++I) {
----------------
Use range-based loop
================
Comment at: lib/Parse/ParseOpenMP.cpp:44
+ OMPD_target_teams_distribute_parallel,
+ OMPD_mapper
};
----------------
Add comma after the new enum
================
Comment at: lib/Parse/ParseOpenMP.cpp:571
+ }
+ if (Clauses.size() == 0) {
+ Diag(Tok, diag::err_omp_expected_clause)
----------------
`Clauses.empty()`
================
Comment at: lib/Sema/SemaOpenMP.cpp:13427
+ TypeResult ParsedType) {
+ assert(ParsedType.isUsable());
+
----------------
Text message
================
Comment at: lib/Sema/SemaOpenMP.cpp:13430
+ QualType MapperType = GetTypeFromParser(ParsedType.get());
+ assert(!MapperType.isNull());
+
----------------
Add the text message for the assert
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56326/new/
https://reviews.llvm.org/D56326
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
