This part is getting closer, and I appreciate that it is smaller, but it's
sliced in a way that isn't testable. The patch needs to go deep on one clause.
For example, there are ASTs for 8 different OpenMP clauses, with printing and
serialization, but there is no way to actually construct those ASTs. Hence,
there are no tests at all.
As I suggested before, take one clause and go far enough that you can parse
it, check some (easy) semantics, and create an AST for it. Then future patches
can introduce additional clauses.
================
Comment at: include/clang/AST/StmtOpenMP.h:144
@@ +143,3 @@
+ /// \brief Return condition.
+ Expr *getCondition() { return dyn_cast_or_null<Expr>(Condition); }
+
----------------
Wei Pan wrote:
> The constructors assume thatn E is not null. Either change the constructor or
> update the get method.
More directly, why are we still storing these as Stmt*? They can be Expr*,
since we no longer have to have Stmt* to deal with child_iterator baggage.
================
Comment at: include/clang/AST/StmtOpenMP.h:190
@@ +189,3 @@
+ /// \brief Return number of threads.
+ Expr *getNumThreads() { return dyn_cast_or_null<Expr>(NumThreads); }
+
----------------
Wei Pan wrote:
> The same as above.
Me too :)
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:2280
@@ +2279,3 @@
+ bool Visit##Class(Class *S) { \
+ for (Stmt::child_range Range = S->children(); Range; ++Range) { \
+ if (!Visitor->TraverseStmt(*Range)) return false; \
----------------
I wonder... rather than propagating the children() anti-pattern from Stmt/Expr,
should we just implement the various Visit* methods directly in the
RecursiveOMPClauseVisitor?
================
Comment at: lib/Basic/OpenMPKinds.cpp:53
@@ +52,3 @@
+
+const char *clang::getOpenMPClauseName(OpenMPClauseKind Kind) {
+ assert(Kind < NUM_OPENMP_CLAUSES);
----------------
This isn't actually used right now. What's it's purpose? It would be fine if
it's part of debugging, but generally we avoid splicing strings like this into
diagnostics (for example).
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:2272
@@ +2271,3 @@
+template <class T>
+class RecursiveOMPClauseVisitor :
+ public OMPClauseVisitor<RecursiveOMPClauseVisitor<T>, bool> {
----------------
Why not just fold this functionality into the RecursiveASTVisitor?
http://llvm-reviews.chandlerc.com/D572
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
