[PATCH] OpenMP parallel directive
Dmitri Gribenko
gribozavr at gmail.com
Sat Apr 20 07:10:53 PDT 2013
================
Comment at: include/clang/AST/StmtOpenMP.h:136
@@ +135,3 @@
+ OMPIfClause(Expr *E, SourceLocation StartLoc, SourceLocation EndLoc)
+ : OMPClause(OMPC_if, StartLoc, EndLoc), Condition(cast<Stmt>(E)) { }
+
----------------
`cast<Stmt>` is not needed here. You could effectively replace this with `assert(E);`
================
Comment at: include/clang/AST/StmtOpenMP.h:127
@@ +126,3 @@
+ ///
+ void setCondition(Expr *E) { Condition = cast<Stmt>(E); }
+public:
----------------
`cast` is not needed here.
================
Comment at: include/clang/AST/StmtOpenMP.h:144
@@ +143,3 @@
+ /// \brief Return condition.
+ Expr *getCondition() { return dyn_cast_or_null<Expr>(Condition); }
+
----------------
Doug Gregor wrote:
> Wei Pan wrote:
> > The constructors assume thatn E is not null. Either change the constructor or update the get method.
> More directly, why are we still storing these as Stmt*? They can be Expr*, since we no longer have to have Stmt* to deal with child_iterator baggage.
Should this member function be const-qualified?
================
Comment at: include/clang/AST/StmtOpenMP.h:32-35
@@ +31,6 @@
+class OMPClause {
+ /// \brief Starting location of the clause.
+ SourceLocation StartLoc;
+ /// \brief Ending location of the clause.
+ SourceLocation EndLoc;
+ /// \brief Kind of the clause.
----------------
Please clarify what these locations are. We probably need locations for: clause keyword, open paren, close paren.
================
Comment at: include/clang/AST/StmtOpenMP.h:58-60
@@ +57,5 @@
+
+ static bool classof(const OMPClause *T) {
+ return true;
+ }
+
----------------
I could be wrong, but IIRC this is not actually needed. dyn_cast magic should figure this out, I think.
================
Comment at: include/clang/AST/StmtOpenMP.h:213-214
@@ +212,4 @@
+ OpenMPDefaultClauseKind Kind;
+ /// \brief Start location of the kind in cource code.
+ SourceLocation KindLoc;
+
----------------
Maybe `KindKwLoc`?
================
Comment at: include/clang/AST/StmtOpenMP.h:362-363
@@ +361,4 @@
+ ///
+ static OMPFirstPrivateClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
This fits in 80 cols.
================
Comment at: include/clang/AST/StmtOpenMP.h:419-420
@@ +418,4 @@
+ ///
+ static OMPSharedClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
This fits in 80 cols.
================
Comment at: include/clang/AST/StmtOpenMP.h:190
@@ +189,3 @@
+ /// \brief Return number of threads.
+ Expr *getNumThreads() { return dyn_cast_or_null<Expr>(NumThreads); }
+
----------------
Doug Gregor wrote:
> Wei Pan wrote:
> > The same as above.
> Me too :)
Also, const-qualified?
================
Comment at: include/clang/AST/StmtOpenMP.h:303-304
@@ +302,4 @@
+ ///
+ static OMPPrivateClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
This fits in 80 cols.
================
Comment at: include/clang/AST/StmtOpenMP.h:503
@@ +502,3 @@
+ OpenMPReductionClauseOperator Operator;
+ /// \brief Start location of the kind in cource code.
+ SourceLocation OperatorLoc;
----------------
Wei Pan wrote:
> location of the reduction operator kind.
Actually, it is just 'location of the reduction operator'.
================
Comment at: include/clang/AST/StmtOpenMP.h:563-567
@@ +562,7 @@
+
+ /// \brief Fetches operator for the clause.
+ OpenMPReductionClauseOperator getOperator() const { return Operator; }
+
+ /// \brief Fetches location of clause operator.
+ SourceLocation getOperatorLoc() const { return OperatorLoc; }
+
----------------
Bikeshedding: s/Fetches/Returns/ everywhere?
These are more or less synonyms, but 'returns' is typically used where the value being returned is not being computed or fetched from somewhere else.
================
Comment at: include/clang/AST/StmtOpenMP.h:627-630
@@ +626,6 @@
+ OpenMPDirectiveKind Kind;
+ /// \brief Starting location of the directive kind.
+ SourceLocation StartLoc;
+ /// \brief Ending location of the directive.
+ SourceLocation EndLoc;
+ /// \brief The statement associated with the directive.
----------------
Please clarify where these point to (omp keyword? directive keyword -- like parallel? #pragma?)
================
Comment at: include/clang/AST/StmtOpenMP.h:645
@@ +644,3 @@
+ /// \param N Number of clauses.
+ /// \param ClausesAndStmt A pointer to the buffer for clauses.
+ ///
----------------
There's no 'ClausesAndStmt' parameter in the function declaration.
Also, ArrayRef?
================
Comment at: include/clang/AST/StmtOpenMP.h:633-636
@@ +632,6 @@
+ Stmt *AssociatedStmt;
+ /// \brief Number of clauses.
+ unsigned NumClauses;
+ /// \brief Pointer to the list of clauses.
+ OMPClause ** const Clauses;
+protected:
----------------
ArrayRef?
================
Comment at: include/clang/AST/StmtOpenMP.h:665
@@ +664,3 @@
+ ///
+ /// \brief Clauses The list of clauses for the directive.
+ ///
----------------
\param CL
================
Comment at: include/clang/AST/StmtOpenMP.h:671
@@ +670,3 @@
+ ///
+ /// /param S Associated statement.
+ ///
----------------
'\param' with backslash -- but you can drop the \param altogether, it is obvious from \brief.
================
Comment at: include/clang/AST/StmtOpenMP.h:701
@@ +700,3 @@
+ ///
+ OMPClause *getClause(unsigned i) {
+ assert(i < NumClauses && "Wrong number of clause!");
----------------
Do we need the non-const overload at all? It is identical to the const overload below.
================
Comment at: include/clang/AST/StmtOpenMP.h:716
@@ +715,3 @@
+ /// \brief Return statement associated with the directive.
+ Stmt *getAssociatedStmt() {
+ return AssociatedStmt;
----------------
Do we need the non-const overload?
================
Comment at: include/clang/AST/StmtOpenMP.h:735
@@ +734,3 @@
+
+ ArrayRef<OMPClause *> clauses() {
+ return getClauses();
----------------
Drop the non-const overload?
Also, why have this 'alias' for getClauses() anyway? The name is against coding standard.
================
Comment at: include/clang/Basic/OpenMPKinds.def:46
@@ +45,3 @@
+
+// Clauses allowed for OpenMP directives
+OPENMP_PARALLEL_CLAUSE(if)
----------------
'.' at the end.
================
Comment at: lib/AST/Stmt.cpp:1140
@@ +1139,3 @@
+ switch(getClauseKind()) {
+ default : break;
+#define OPENMP_CLAUSE(Name, Class) \
----------------
No space before ':'.
Also, do we need default at all? It silences useful compiler warnings.
================
Comment at: lib/AST/Stmt.cpp:1139
@@ +1138,3 @@
+StmtRange OMPClause::children() {
+ switch(getClauseKind()) {
+ default : break;
----------------
Space before '('
================
Comment at: lib/AST/StmtPrinter.cpp:587
@@ +586,3 @@
+namespace {
+class OMPClausePrinter : public OMPClauseVisitor<OMPClausePrinter> {
+ StmtPrinter *Printer;
----------------
Make sure to add tests for printing when parsing and semantic analysis is implemented.
================
Comment at: lib/Basic/OpenMPKinds.cpp:63-64
@@ +62,4 @@
+ return "threadprivate or thread local";
+ default:
+ break;
+ }
----------------
'default' should not be needed -- here and in other functions in this file.
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1616-1617
@@ +1615,4 @@
+ break;
+ default:
+ return 0;
+ }
----------------
Is this possible at all?
http://llvm-reviews.chandlerc.com/D572
More information about the cfe-commits
mailing list