[PATCH] OpenMP parallel directive

[Doug Gregor]

  I suggest pulling out the "structured block" checking, which is untested and looks very incomplete. Otherwise, very minor changes requested this time around, then this can go in.


================
Comment at: include/clang/AST/StmtOpenMP.h:80
@@ +79,3 @@
+  /// \brief Fetches list of variables associated with this clause.
+  llvm::MutableArrayRef<Expr *> getVars() {
+    return llvm::MutableArrayRef<Expr *>(
----------------
getVars() is slightly misleading. getVarRefs(), perhaps?

================
Comment at: include/clang/AST/StmtOpenMP.h:169
@@ +168,3 @@
+  ///
+  explicit OMPDefaultClause()
+    : OMPClause(OMPC_default, SourceLocation(), SourceLocation()),
----------------
This doesn't need to be 'explicit'.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6408
@@ +6407,3 @@
+def err_omp_private_incomplete_type : Error<
+  "a private variable with incomplete type %0">;
+def err_omp_directive_nonblock : Error<
----------------
Please drop the leading "a " from the diagnostic.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6418
@@ -6407,1 +6417,3 @@
+def err_omp_clause_ref_type_arg : Error<
+  "arguments of OpenMP clause '%0' cannot be of reference type">;
 def err_omp_threadprivate_incomplete_type : Error<
----------------
How about adding the actual type (via %1) at the end of this? It can be important when dealing with template instantiations.

================
Comment at: lib/Sema/SemaOpenMP.cpp:210
@@ +209,3 @@
+    //return false;
+    return true;
+  }
----------------
Why is this always returning true?

================
Comment at: lib/Sema/SemaOpenMP.cpp:202
@@ +201,3 @@
+namespace {
+class StructuredStmtChecker : public StmtVisitor<StructuredStmtChecker, bool> {
+public:
----------------
I gather that this is trying to check whether the given statement is a structured block, but it's not clear exactly what rules are being followed. (The OpenMP Specification is ridiculously vague in its definition). Note that the checking for "goto" rules are elsewhere (as they should be), and we aren't trying to check the rules for "throw" (which we can't figure out statically anyway), or "setjmp" (which could be here).

================
Comment at: lib/Sema/SemaOpenMP.cpp:255
@@ +254,3 @@
+    if (!Checker.Visit(AStmt)) {
+      Diag(StartLoc, diag::err_omp_directive_nonblock)
+        << getOpenMPDirectiveName(Kind) << AStmt;
----------------
This isn't a very helpful diagnostic, because it doesn't show which part of the statement makes it not a structured block. Also, there don't seem to be any tests for this diagnostic.

I suggest removing the structured-block checking from this patch, because it's not nearly as baked as the rest of the patch and I don't want it holding up the rest.

================
Comment at: lib/Sema/SemaOpenMP.cpp:422
@@ +421,3 @@
+      if (!CD ||
+          CheckConstructorAccess(ELoc, CD,
+                                 InitializedEntity::InitializeTemporary(Type),
----------------
Should also check that CD is not deleted and somewhere check for unavailable/deprecated (e.g., via DiagnoseUseOfDecl). I suspect you'll end up building a call to the default constructor to stash in the AST, but that doesn't need to be part of this patch.



================
Comment at: lib/Sema/SemaOpenMP.cpp:436
@@ +435,3 @@
+      CXXDestructorDecl *DD = RD->getDestructor();
+      if (DD && !DD->isTrivial()) {
+        if (CheckDestructorAccess(ELoc, DD, PD) == AR_inaccessible) {
----------------
Why the trivial check?

================
Comment at: lib/Sema/SemaOpenMP.cpp:380
@@ +379,3 @@
+    //  structure element) cannot appear in a private clause.
+    DeclRefExpr *DE = dyn_cast_or_null<DeclRefExpr>(*I);
+    if (!DE || !isa<VarDecl>(DE->getDecl())) {
----------------
It doesn't have to be handled as part of this patch, but within templates you'll also need to handle DependentScopeDeclRefExprs, as well have VD->getType() being type-dependent.

================
Comment at: lib/Serialization/ASTWriterStmt.cpp:1713
@@ +1712,3 @@
+void ASTStmtWriter::VisitOMPParallelDirective(OMPParallelDirective *D) {
+  VisitOMPExecutableDirective(D);
+  Code = serialization::STMT_OMP_PARALLEL_DIRECTIVE;
----------------
There are no tests for this code?


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