r209660 - Parsing/Sema for OMPCollapseClause.

[Samuel F Antao]

Hi,

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.

-- Samuel


> 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.
> Message-ID:
>    <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
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/4827fbb6/attachment.html>