<html><body>
<p><tt><font size="2">Hi,</font></tt><br>
<br>
<tt><font size="2">I reviewed the commit r209660 for the OpenMP collapse clause and it looks good for me. </font></tt><br>
<br>
<tt><font size="2">It follows what is already done for other clauses like safelen, so I do</font></tt><br>
<tt><font size="2">not have any particular remark. I tried the test cases for this clause, which are comprehensive, and I also tried </font></tt><br>
<tt><font size="2">some code I wrote down myself. The output is the expected.</font></tt><br>
<tt><font size="2"> </font></tt><br>
<tt><font size="2">-- Samuel</font></tt><br>
<br>
<tt><font size="2"><br>
> Date: Tue, 3 Jun 2014 11:11:40 -0400<br>
> From: Carlo Bertolli <cbertol@us.ibm.com><br>
> To: cfe-commits@cs.uiuc.edu<br>
> Subject: Re: r209660 - Parsing/Sema for OMPCollapseClause.<br>
> Message-ID:<br>
> <OFE756CB1E.164BF76B-ON85257CEC.0050BF41-85257CEC.005377C7@us.ibm.com><br>
> Content-Type: text/plain; charset="utf-8"<br>
> <br>
> <br>
> Hi Alexander,<br>
> <br>
> I reviewed the code you put down for the OMP collapse clause (as in<br>
> subject: r209660).<br>
> <br>
> It looks fine to me - I only have some minor aesthetic comments:<br>
> <br>
> <br>
> 1. In OpenMPClause.h: Stmt * NumForLoops ?> why not a Expr *? It cannot be<br>
> anything else.<br>
> Is this because we need a Stmt * to be passed to the RecursiveASTVisitor<br>
> function?<br>
> <br>
> 2. In Sema.h, ActOnOpenMPCollapseClause: Expr * Num variable. Consider<br>
> making it more explicit, as ?NumForLoops?.<br>
> <br>
> 3. In SemaOpenMP.cpp, Sema::ActOnOpenMPCollapseClause, the comment seems to<br>
> suggest that only the simd construct has the collapse clause, while also<br>
> ?for? and ?distribute: can have it. The related sections in the OpenMP<br>
> specifications are: 2.7.1 and 2.9.6. This will have to be fixed once ?for?<br>
> and ?distribute? get pushed from github into the main trunk.<br>
> <br>
> 4. TreeTransform.h: all comments to ?Rebuild*? functions are the same.<br>
> Consider specializing (what semantic analysis) if appropriate or have a<br>
> single comment? Similarly, there are no comments at all on the ?Transform*?<br>
> functions. Consider adding a single comment for all of them. The<br>
> indentation of ?TransformOMPCollapseClause? is different from the majority<br>
> of the other clauses. Consider making it homogeneous.<br>
> <br>
> <br>
> Thanks<br>
> <br>
> -- Carlo<br>
</font></tt></body></html>