r209660 - Parsing/Sema for OMPCollapseClause.
Samuel F Antao
sfantao at us.ibm.com
Tue Jun 3 16:38:39 PDT 2014
I reviewed the commit r209660 for the OpenMP collapse clause and it looks
good for me.
It follows what is already done for other clauses like safelen, so I do
not have any particular remark. I tried the test cases for this clause,
which are comprehensive, and I also tried
some code I wrote down myself. The output is the expected.
> Date: Tue, 3 Jun 2014 11:11:40 -0400
> From: Carlo Bertolli <cbertol at us.ibm.com>
> To: cfe-commits at cs.uiuc.edu
> Subject: Re: r209660 - Parsing/Sema for OMPCollapseClause.
> <OFE756CB1E.164BF76B-ON85257CEC.0050BF41-85257CEC.005377C7 at us.ibm.com>
> Content-Type: text/plain; charset="utf-8"
> 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
> anything else.
> Is this because we need a Stmt * to be passed to the RecursiveASTVisitor
> 2. In Sema.h, ActOnOpenMPCollapseClause: Expr * Num variable. Consider
> making it more explicit, as ?NumForLoops?.
> 3. In SemaOpenMP.cpp, Sema::ActOnOpenMPCollapseClause, the comment seems
> 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
> 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
> functions. Consider adding a single comment for all of them. The
> indentation of ?TransformOMPCollapseClause? is different from the
> of the other clauses. Consider making it homogeneous.
> -- Carlo
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits