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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
