Doug, I'm sorry for such big and complex patch. I understand that is very
problematic for you, Dmitri, Hal and others to review such large patches. But
currently it's quite hard to separate this patch and the next one (with
'#pragma omp for' support, which is smaller, but still quite big) into several
small patches. I'll try to make next patches much smaller and more convenient
for review.
================
Comment at: include/clang/AST/StmtOpenMP.h:72
@@ +71,3 @@
+/// clauses in the '#pragma omp ...' directive.
+class OMPSingleExpr {
+ /// \brief Expression for the clause.
----------------
Doug Gregor wrote:
> 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.
Doug, Ok, I'll remove it.
================
Comment at: include/clang/AST/StmtOpenMP.h:94
@@ +93,3 @@
+template <typename T>
+class OMPSimple {
+ /// \brief Argument of the clause.
----------------
Doug Gregor wrote:
> 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.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:132
@@ +131,3 @@
+ /// \brief List of references to variables.
+ Expr **Vars;
+protected:
----------------
Doug Gregor wrote:
> 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.
You're right. That's the better solution.
================
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<
----------------
Doug Gregor wrote:
> How about simple "private variable with incomplete type %0", like we do
> elsewhere in Clang? The same comment for the next few diagnostics.
Ok.
================
Comment at: lib/Parse/ParseOpenMP.cpp:71
@@ +70,3 @@
+/// threadprivate-directive
+/// annot_pragma_openmp threadprivate simple-variable-list
+/// annot_pragma_openmp_end
----------------
Doug Gregor wrote:
> threadprivate is a keyword, so it should be in 'single quotes'
Ok, I'll fix all comments
================
Comment at: lib/Parse/ParseOpenMP.cpp:138
@@ +137,3 @@
+ Stmt *AStmt = AssociatedStmt.take();
+ AssociatedStmt = Actions.ActOnCompoundStmt(AStmt->getLocStart(),
+ AStmt->getLocEnd(),
----------------
Doug Gregor wrote:
> If we're just parsing one statement, why are we wrapping it up in a
> CompoundStmt?
Actually, I want to use the same code for all directives. By there is at least
one directive (#pragma omp for), which could have several associated for-loops.
For example:
#pragma omp for collapse(2)
for(..) {
}
for(...) {
}
These 2 loops will be associated with the '#pragma omp for'.
================
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();
----------------
Doug Gregor wrote:
> This recovery seems overly aggressive. Why not still parse the clause, but
> have the caller ignore it?
Ok. I'll fix it.
================
Comment at: lib/Parse/ParseOpenMP.cpp:372
@@ +371,3 @@
+
+ if (LParen && Tok.isNot(tok::r_paren)) {
+ Diag(Tok, diag::err_expected_rparen);
----------------
Doug Gregor wrote:
> You can use the BalancedDelimiterTracker to automate some of these
> diagnostics.
Nope, I can't. Actually I tried to use it. It uses ';' as the stop point for
parsing, but I have to stop parsing of the directive at
'tok::annot_pragma_openmp_end'. This class makes parsing very complex, because
it skips all annotation tokens for OpenMP directives.
================
Comment at: lib/Sema/SemaOpenMP.cpp:34
@@ +33,3 @@
+
+static bool isConstant(ASTContext &Context, QualType Type) {
+ CanQualType CT = Context.getCanonicalType(nonRefType(Type));
----------------
Doug Gregor wrote:
> Please use QualType::isContext() directly.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:59
@@ +58,3 @@
+ static bool classof(const Stmt *T) {
+ return T->getStmtClass() == OMPNumThreadsClauseClass ||
+ T->getStmtClass() == OMPIfClauseClass ||
----------------
Doug Gregor wrote:
> Can we use a more-efficient >= && <= comparison here?
I'll try to fix it.
================
Comment at: include/clang/AST/StmtOpenMP.h:31
@@ +30,3 @@
+///
+class OMPClause : public Stmt {
+ /// \brief Starting location of the clause.
----------------
Doug Gregor wrote:
> 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.
Ok, I'll fix it.
================
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">;
----------------
Doug Gregor wrote:
> Aside: can we introduce a new category for OpenMP diagnostics?
I'll try to make it.
================
Comment at: lib/Parse/ParseOpenMP.cpp:70
@@ +69,3 @@
+///
+/// threadprivate-directive
+/// annot_pragma_openmp threadprivate simple-variable-list
----------------
Doug Gregor wrote:
> Technically missing a ':' after threadprivate-directive
Ok.
================
Comment at: lib/Parse/ParseOpenMP.cpp:122
@@ +121,3 @@
+ // Skip ',' if any
+ if (Tok.is(tok::comma))
+ ConsumeToken();
----------------
Doug Gregor wrote:
> Are the commas actually optional?
Yes, commas are optional. For example, here is the syntax of #pragma omp
parallel from standard document:
#pragma omp parallel [clause[ [, ]clause] ...] new-line
As you can see, comma is optional.
================
Comment at: lib/Sema/SemaOpenMP.cpp:30
@@ -22,1 +29,3 @@
+static QualType nonRefType(QualType Type) {
+ return Type.getNonReferenceType();
----------------
Doug Gregor wrote:
> Please don't add this. Use QualType::getNonReferenceType() directly.
Ok.
http://llvm-reviews.chandlerc.com/D572
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
