[PATCH] OpenMP parallel directive and qualified names in threadprivate

[Doug Gregor]

  This patch is way too large. Ideally, it would be something like:

    - Basics of OpenMP clauses with one or two clauses (to get the design right)
    - Series of patches with the other OpenMP clauses + their parsing and semantic analysis
    - OpenMP directives with parsing
    - Jump scope checking
    - Data sharing attributes stack


================
Comment at: include/clang/AST/StmtOpenMP.h:72
@@ +71,3 @@
+/// clauses in the '#pragma omp ...' directive.
+class OMPSingleExpr {
+  /// \brief Expression for the clause.
----------------
This base class just doesn't seem worth it. It's only abstracting over the storage for a single expression, which I'd rather just see in the various subclasses of OMPSingleExpr. Plus, this abstraction has caused the weird getExpr() signature, which returns the address of the underlying expression rather than just the expression.

Conventionally, we'd store this as a Stmt* and cast to/from Expr* in the accessor and mutator.

================
Comment at: include/clang/AST/StmtOpenMP.h:94
@@ +93,3 @@
+template <typename T>
+class OMPSimple {
+  /// \brief Argument of the clause.
----------------
Similar to OMPSingleExpr, this just doesn't seem worth it, and it's an odd use of multiple inheritance.

Plus, it means that all of the subclasses have the generic name "getArgument()" for their accessor, as well as having a mutator they don't need. We should have more descriptive accessor names, and mutation (which only happens in the ASTReader) should be handled directly by friend'ing the reader.

================
Comment at: include/clang/AST/StmtOpenMP.h:59
@@ +58,3 @@
+  static bool classof(const Stmt *T) {
+    return T->getStmtClass() == OMPNumThreadsClauseClass ||
+           T->getStmtClass() == OMPIfClauseClass ||
----------------
Can we use a more-efficient >= && <= comparison here?

================
Comment at: include/clang/AST/StmtOpenMP.h:31
@@ +30,3 @@
+///
+class OMPClause : public Stmt {
+  /// \brief Starting location of the clause.
----------------
Why is an OMPClause a Stmt? These aren't executable code of any kind, nor are they expressions in the C sense. It makes sense to have an OMPClause and its subclasses, but I think it needs to be a standalone hierarchy. That means a little more work in the RecursiveASTVisitor, but it's the right solution.

================
Comment at: include/clang/AST/StmtOpenMP.h:132
@@ +131,3 @@
+  /// \brief List of references to variables.
+  Expr **Vars;
+protected:
----------------
Part of the reason for tail-allocating the list of variable references is to eliminate this pointer from the AST. If we're going to inherit from OMPVarList to get this information, I suggest templating it on the subclass, so we can downcast to that class and use "this+1" as the address of the list of variable references.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6201
@@ -6200,3 +6200,3 @@
 // OpenMP support.
 def err_omp_expected_var_arg_suggest : Error<
   "%0 is not a global variable, static local variable or static data member%select{|; did you mean %2?}1">;
----------------
Aside: can we introduce a new category for OpenMP diagnostics?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6216
@@ +6215,3 @@
+def err_omp_private_incomplete_type : Error<
+  "a private variable must not have incomplete type %0">;
+def err_omp_firstprivate_incomplete_type : Error<
----------------
How about simple "private variable with incomplete type %0", like we do elsewhere in Clang? The same comment for the next few diagnostics.

================
Comment at: lib/Parse/ParseOpenMP.cpp:70
@@ +69,3 @@
+///
+///       threadprivate-directive
+///         annot_pragma_openmp threadprivate simple-variable-list
----------------
Technically missing a ':' after threadprivate-directive

================
Comment at: lib/Parse/ParseOpenMP.cpp:71
@@ +70,3 @@
+///       threadprivate-directive
+///         annot_pragma_openmp threadprivate simple-variable-list
+///         annot_pragma_openmp_end
----------------
threadprivate is a keyword, so it should be in 'single quotes'

================
Comment at: lib/Parse/ParseOpenMP.cpp:122
@@ +121,3 @@
+      // Skip ',' if any
+      if (Tok.is(tok::comma))
+        ConsumeToken();
----------------
Are the commas actually optional?

================
Comment at: lib/Parse/ParseOpenMP.cpp:138
@@ +137,3 @@
+        Stmt *AStmt = AssociatedStmt.take();
+        AssociatedStmt = Actions.ActOnCompoundStmt(AStmt->getLocStart(),
+                                                   AStmt->getLocEnd(),
----------------
If we're just parsing one statement, why are we wrapping it up in a CompoundStmt?

================
Comment at: lib/Parse/ParseOpenMP.cpp:258
@@ +257,3 @@
+      << getOpenMPClauseName(CKind) << getOpenMPDirectiveName(DKind);
+    SkipUntil(tok::comma, tok::annot_pragma_openmp_end, false, true);
+    return StmtError();
----------------
This recovery seems overly aggressive. Why not still parse the clause, but have the caller ignore it?

================
Comment at: lib/Parse/ParseOpenMP.cpp:372
@@ +371,3 @@
+
+  if (LParen && Tok.isNot(tok::r_paren)) {
+    Diag(Tok, diag::err_expected_rparen);
----------------
You can use the BalancedDelimiterTracker to automate some of these diagnostics.

================
Comment at: lib/Sema/SemaOpenMP.cpp:30
@@ -22,1 +29,3 @@
 
+static QualType nonRefType(QualType Type) {
+  return Type.getNonReferenceType();
----------------
Please don't add this. Use QualType::getNonReferenceType() directly.

================
Comment at: lib/Sema/SemaOpenMP.cpp:34
@@ +33,3 @@
+
+static bool isConstant(ASTContext &Context, QualType Type) {
+  CanQualType CT = Context.getCanonicalType(nonRefType(Type));
----------------
Please use QualType::isContext() directly.


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