r209660 - Parsing/Sema for OMPCollapseClause.

[Alexander Musman]

Hi Carlo, Samuel,

Thank you for review.

Carlo, I've just committed a fix for most of your comments in r. 210169,
here are my answers:
 > 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?

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.


> 2. In Sema.h, ActOnOpenMPCollapseClause: Expr * Num variable. Consider
making it more explicit, as “NumForLoops”.

Fixed.


> 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.

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.


> 4. TreeTransform.h: all comments to “Rebuild*” functions are the same.
Consider specializing (what semantic analysis) if appropriate or have a
single comment?
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.


> 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.

OK - I added a group comment and reformatted OpenMP Transform-functions
with clang-format.


Regards,
Alexander



2014-06-04 3:38 GMT+04:00 Samuel F Antao <sfantao at us.ibm.com>:

> 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/20140604/9cf77a04/attachment.html>