<div dir="ltr"><div>Hi Carlo, Samuel,</div><div><br></div><div>Thank you for review.</div><div><br></div><div>Carlo, I've just committed a fix for most of your comments in r. 210169, here are my answers:</div><div><span class="" style="white-space:pre"> </span></div>
<div><span class="" style="white-space:pre"> </span>> 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?</div>
<div><br></div><div>This is for consistency with previous clauses, otherwise it seems no much difference between using Expr and Stmt here. For example, if we choose Expr *, it would need cast to Stmt * in the children() method.</div>
<div><br></div><div><br></div><div>><span class="" style="white-space:pre"> </span>2. In Sema.h, ActOnOpenMPCollapseClause: Expr * Num variable. Consider making it more explicit, as “NumForLoops”.</div><div><br></div><div>
Fixed.</div><div><br></div><div><br></div><div>><span class="" style="white-space:pre"> </span>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.</div>
<div><br></div><div>Sure, this clause (and all iterations spaces and collapsing-related code) will be allowed for all loop directives. I think it's OK to fix comment beforehand.</div><div><br></div><div><br></div><div>
><span class="" style="white-space:pre"> </span>4. TreeTransform.h: all comments to “Rebuild*” functions are the same. Consider specializing (what semantic analysis) if appropriate or have a single comment? </div><div>
It turns out that they were also not quite exact, because OpenMP clause is built. I've updated them for now to refer to clause.</div><div><br></div><div><br></div><div>><span class="" style="white-space:pre"> </span>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.</div>
<div><br></div><div>OK - I added a group comment and reformatted OpenMP Transform-functions with clang-format.</div><div><br></div><div><br></div><div>Regards,</div><div>Alexander</div><div><br></div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">2014-06-04 3:38 GMT+04:00 Samuel F Antao <span dir="ltr"><<a href="mailto:sfantao@us.ibm.com" target="_blank">sfantao@us.ibm.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<p><tt><font>Hi,</font></tt><br>
<br>
<tt><font>I reviewed the commit r209660 for the OpenMP collapse clause and it looks good for me. </font></tt><br>
<br>
<tt><font>It follows what is already done for other clauses like safelen, so I do</font></tt><br>
<tt><font>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>some code I wrote down myself. The output is the expected.</font></tt><br>
<tt><font> </font></tt><br>
<tt><font>-- Samuel</font></tt><br>
<br>
<tt><font><br>
> Date: Tue, 3 Jun 2014 11:11:40 -0400<br>
> From: Carlo Bertolli <<a href="mailto:cbertol@us.ibm.com" target="_blank">cbertol@us.ibm.com</a>><br>
> To: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> Subject: Re: r209660 - Parsing/Sema for OMPCollapseClause.<br>
> Message-ID:<br>
> <<a href="mailto:OFE756CB1E.164BF76B-ON85257CEC.0050BF41-85257CEC.005377C7@us.ibm.com" target="_blank">OFE756CB1E.164BF76B-ON85257CEC.0050BF41-85257CEC.005377C7@us.ibm.com</a>><br>
> Content-Type: text/plain; charset="utf-8"</font></tt></p><div class=""><tt><font><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></font></tt></div><tt><font>
> making it more explicit, as ?NumForLoops?.<div class=""><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></div>
> ?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.<div class=""><br>
> Consider specializing (what semantic analysis) if appropriate or have a<br></div>
> single comment? Similarly, there are no comments at all on the ?Transform*?<div class=""><br>
> functions. Consider adding a single comment for all of them. The<br></div>
> indentation of ?TransformOMPCollapseClause? is different from the majority<div class=""><br>
> of the other clauses. Consider making it homogeneous.<br>
> <br>
> <br>
> Thanks<br>
> <br>
> -- Carlo<br>
</div></font></tt><p></p></div></blockquote></div><br></div>