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 <[email protected]>: > 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 <[email protected]> > > To: [email protected] > > Subject: Re: r209660 - Parsing/Sema for OMPCollapseClause. > > Message-ID: > > <ofe756cb1e.164bf76b-on85257cec.0050bf41-85257cec.00537...@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 > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
