[PATCH] OpenMP parallel directive

[Dmitri Gribenko]

================
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