[PATCH] OpenMP parallel directive and qualified names in threadprivate

[Alexey Bataev]

================
Comment at: include/clang/AST/StmtOpenMP.h:31
@@ +30,3 @@
+///
+class OMPClause : public Stmt {
+  /// \brief Starting location of the clause.
----------------
Dmitri Gribenko wrote:
> 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...`
Dmitri, it is a top-level entity only for clauses. For executable directives there is another basic class - OMPExecutableDirective.

================
Comment at: include/clang/AST/StmtOpenMP.h:78
@@ +77,3 @@
+///
+class OMPClauseList : public OMPClause {
+  friend class ASTStmtReader;
----------------
Dmitri Gribenko wrote:
> Is inheritance the correct tool here?  It doesn't make a lot of sense for me to derive "a list of T" from T...
> 
Yes, it seems you're right. I'll fix it.

================
Comment at: include/clang/AST/StmtOpenMP.h:146
@@ +145,3 @@
+///
+class OMPSingleExprClause : public OMPClause {
+  /// \brief Expression for the clause.
----------------
Dmitri Gribenko wrote:
> 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.
> 
Ok, I'l add OMPIfClause and OMPNumThreadsClause

================
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.
----------------
Dmitri Gribenko wrote:
> Maybe change the type of ClauseExpr to `Expr *`?
> 
Ok

================
Comment at: include/clang/AST/StmtOpenMP.h:195
@@ +194,3 @@
+///
+class OMPSimpleClause : public OMPClause {
+  /// \brief Internal type of the clause.
----------------
Dmitri Gribenko wrote:
> 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.
Ok, I'll add OMPDefaultClause. As to OMPReductionClause, it is different kind of clauses. I think it should be derived from OMPVarListClause

================
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.
----------------
Dmitri Gribenko wrote:
> The term 'type' is very overloaded in Clang.  Can we call this "argument" instead?
> 
Ok

================
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.
----------------
Dmitri Gribenko wrote:
> When you split the class, this should be changed from 'unsigned' to an enumeration type.
Ok

================
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.
----------------
Dmitri Gribenko wrote:
> I think that `reduction` is special with its semicolon in between (`reduction(op:vars)`), so it should implement the variable list functionality by itself.
I think OMPVarListClause could be a base class for reduction.

================
Comment at: include/clang/AST/StmtOpenMP.h:258
@@ +257,3 @@
+///
+class OMPVarListClause : public OMPClause {
+  friend class ASTStmtReader;
----------------
Dmitri Gribenko wrote:
> 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.
Ok

================
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 *>(
----------------
Dmitri Gribenko wrote:
> Why `Stmt*` here?  (and in other getVars() overload)
I'll change to Expr *

================
Comment at: include/clang/AST/StmtOpenMP.h:274-275
@@ +273,4 @@
+  void setVars(ArrayRef<DeclRefExpr *> VL);
+  /// \brief Sets 
+  void setOperator(unsigned Op);
+
----------------
Dmitri Gribenko wrote:
> Incomplete comment.  Can we change the parameter type to enumeration?
Yes, missed the main part. I'll fix it.

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

================
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.
----------------
Dmitri Gribenko wrote:
> 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?
> 
Ok, will be fixed

================
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:
----------------
Dmitri Gribenko wrote:
> 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.
1. It is required for children method to store all associated statements in a single array. See for example CXXForRangeStmt.
2. I'll to rework this representation.

================
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() { }
----------------
Dmitri Gribenko wrote:
> StartLoc, EndLoc are more idiomatic.  (Name shadowing is not an issue.)
> 
> Why specify default arguments?
> 
Ok.
Default arguments are for constructors, which construct empty directives. I'll remove them.


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