[PATCH] OpenMP parallel directive and qualified names in threadprivate

[Dmitri Gribenko]

  Hi Alexey,

  I have a few high-level comments about AST design, so I reviewed just the AST portion (other parts might change significantly if you agree with my comments).


================
Comment at: include/clang/AST/StmtOpenMP.h:204
@@ +203,3 @@
+  /// \brief K Clause kind ('default').
+  /// \brief T Type of the clause ('none' or 'shared').
+  /// \brief TLoc Starting location of the internal type.
----------------
The term 'type' is very overloaded in Clang.  Can we call this "argument" instead?


================
Comment at: include/clang/AST/StmtOpenMP.h:78
@@ +77,3 @@
+///
+class OMPClauseList : public OMPClause {
+  friend class ASTStmtReader;
----------------
Is inheritance the correct tool here?  It doesn't make a lot of sense for me to derive "a list of T" from T...


================
Comment at: include/clang/AST/StmtOpenMP.h:195
@@ +194,3 @@
+///
+class OMPSimpleClause : public OMPClause {
+  /// \brief Internal type of the clause.
----------------
This represents two different clauses: 'default' and 'reduction'.  In my opinion it would be better to split it into two: `OMPDefaultClause` and `OMPReductionClause`, to capture the semantics better.  This also allows us to capture additional syntactic features, too, like the location of ':' in `reduction(op:list)`.

It also avoids stuffing unrelated enumerations into a single 'unsigned' variable, see below.

================
Comment at: include/clang/AST/StmtOpenMP.h:223
@@ +222,3 @@
+  /// \brief Fetches type of the clause.
+  unsigned getClauseType() const { return Type; }
+  /// \brief Set type of the clauses.
----------------
When you split the class, this should be changed from 'unsigned' to an enumeration type.

================
Comment at: include/clang/AST/StmtOpenMP.h:146
@@ +145,3 @@
+///
+class OMPSingleExprClause : public OMPClause {
+  /// \brief Expression for the clause.
----------------
This class represents two unrelated concepts.  While I think it is OK to share implementation via an abstract parent class, you should derive two concrete classes: `OMPIfClause` and `OMPNumThreadsClause` to capture the semantics better.


================
Comment at: include/clang/AST/StmtOpenMP.h:170
@@ +169,3 @@
+  /// \brief Return expression for this clause.
+  Expr *getExpr() const { return cast<Expr>(ClauseExpr); }
+  /// \brief Set the expression for this clause.
----------------
Maybe change the type of ClauseExpr to `Expr *`?


================
Comment at: include/clang/AST/StmtOpenMP.h:31
@@ +30,3 @@
+///
+class OMPClause : public Stmt {
+  /// \brief Starting location of the clause.
----------------
I might be misunderstanding something, but I don't think that the top-level entity should be a single clause.  IMHO, it should be a clause list, or better -- a directive that contains a list of clauses.  This allows us to capture source locations of the keyword 'parallel' in `#pragma omp parallel clause...`

================
Comment at: include/clang/AST/StmtOpenMP.h:248
@@ +247,3 @@
+/// \brief This represents clauses with the list of variables like 'private',
+/// 'firstprivate', 'copyin', 'shared', or 'reduction' clauses in the
+/// '#pragma omp ...' directives.
----------------
I think that `reduction` is special with its semicolon in between (`reduction(op:vars)`), so it should implement the variable list functionality by itself.

================
Comment at: include/clang/AST/StmtOpenMP.h:258
@@ +257,3 @@
+///
+class OMPVarListClause : public OMPClause {
+  friend class ASTStmtReader;
----------------
Similarly to `OMPSingleExprClause`, I'd propose making this class abstract, and deriving concrete classes that represent individual clauses.

This also avoids having a two-level kind encoding scheme: the top-level kind is the usual Stmt kind, and at the second level we have a specific omp kind.

================
Comment at: include/clang/AST/StmtOpenMP.h:266
@@ +265,3 @@
+  /// \brief Fetches the list of variables associated with this clause.
+  llvm::MutableArrayRef<Stmt *> getVars() {
+    return llvm::MutableArrayRef<Stmt *>(
----------------
Why `Stmt*` here?  (and in other getVars() overload)

================
Comment at: include/clang/AST/StmtOpenMP.h:274-275
@@ +273,4 @@
+  void setVars(ArrayRef<DeclRefExpr *> VL);
+  /// \brief Sets 
+  void setOperator(unsigned Op);
+
----------------
Incomplete comment.  Can we change the parameter type to enumeration?

================
Comment at: include/clang/AST/StmtOpenMP.h:322-325
@@ +321,6 @@
+  ///
+  static OMPVarListClause *CreateEmpty(
+                    ASTContext &Context,
+                    OpenMPClauseKind K,
+                    unsigned N, EmptyShell);
+
----------------
You can fit 'Context' on the previous line.

================
Comment at: include/clang/AST/StmtOpenMP.h:381-382
@@ +380,4 @@
+  OMPExecutableDirective(StmtClass SC, OpenMPDirectiveKind K,
+                         SourceLocation SL = SourceLocation(),
+                         SourceLocation EL = SourceLocation())
+    : Stmt(SC), Kind(K), StartLoc(SL), EndLoc(EL), ClausesAndStmt() { }
----------------
StartLoc, EndLoc are more idiomatic.  (Name shadowing is not an issue.)

Why specify default arguments?


================
Comment at: include/clang/AST/StmtOpenMP.h:366-369
@@ +365,6 @@
+  OpenMPDirectiveKind Kind;
+  /// \brief Starting location of the clause.
+  SourceLocation StartLoc;
+  /// \brief Ending location of the clause.
+  SourceLocation EndLoc;
+  /// \brief List of clauses and the statement for the directive.
----------------
Clause?

Also, please be specific about what these locations are.

Is the startloc the location of '#' in '#pragma'?  What is the location of 'parallel' keyword, then?

What is endloc?


================
Comment at: include/clang/AST/StmtOpenMP.h:370-371
@@ +369,4 @@
+  SourceLocation EndLoc;
+  /// \brief List of clauses and the statement for the directive.
+  Stmt *ClausesAndStmt[NUM_OPENMP_CLAUSES];
+protected:
----------------
I don't think this is the correct representation.

(1) the associated stmt could be a separate pointer, it does not belong to the clause list at all.

(2) it forces us to allocate memory for all clause kinds every time.  I think it would be better to tail-allocate a list of pointers to clauses (not of clause lists), and store clauses in source order.


http://llvm-reviews.chandlerc.com/D572