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