
<html><body>


<p><font size="2" face="sans-serif">Hi Alexander,</font><br>


<br>


<font size="2" face="sans-serif">I reviewed the code you put down for the OMP collapse clause (as in subject: r209660).</font><br>


<br>


<font size="2" face="sans-serif">It looks fine to me - I only have some minor aesthetic comments:</font><br>


<br>


<br>


<font size="2" face="sans-serif">1. In OpenMPClause.h: Stmt * NumForLoops —> why not a Expr *? It cannot be anything else.</font><br>


<font size="2" face="sans-serif">Is this because we need a Stmt * to be passed to the RecursiveASTVisitor function?</font><br>


<br>


<font size="2" face="sans-serif">2. In Sema.h, ActOnOpenMPCollapseClause: Expr * Num variable. Consider making it more explicit, as “NumForLoops”.</font><br>


<br>


<font size="2" face="sans-serif">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.</font><br>


<br>


<font size="2" face="sans-serif">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.</font><br>


<br>


<br>


<font size="2" face="sans-serif">Thanks</font><br>


<br>


<font size="2" face="sans-serif">-- Carlo</font><br>


<br>


</body></html>