Hi Alexander, I reviewed the code you put down for the OMP collapse clause (as in subject: r209660).
It looks fine to me - I only have some minor aesthetic comments: 1. In OpenMPClause.h: Stmt * NumForLoops —> why not a Expr *? It cannot be anything else. Is this because we need a Stmt * to be passed to the RecursiveASTVisitor function? 2. In Sema.h, ActOnOpenMPCollapseClause: Expr * Num variable. Consider making it more explicit, as “NumForLoops”. 3. In SemaOpenMP.cpp, Sema::ActOnOpenMPCollapseClause, the comment seems to suggest that only the simd construct has the collapse clause, while also “for” and “distribute: can have it. The related sections in the OpenMP specifications are: 2.7.1 and 2.9.6. This will have to be fixed once “for” and “distribute” get pushed from github into the main trunk. 4. TreeTransform.h: all comments to “Rebuild*” functions are the same. Consider specializing (what semantic analysis) if appropriate or have a single comment? Similarly, there are no comments at all on the “Transform*” functions. Consider adding a single comment for all of them. The indentation of “TransformOMPCollapseClause” is different from the majority of the other clauses. Consider making it homogeneous. Thanks -- Carlo
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
