r209660 - Parsing/Sema for OMPCollapseClause.

[Carlo Bertolli]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140603/80c70d89/attachment.html>