================
Comment at: include/clang/AST/RecursiveASTVisitor.h:2272
@@ +2271,3 @@
+template <class T>
+class RecursiveOMPClauseVisitor :
+ public OMPClauseVisitor<RecursiveOMPClauseVisitor<T>, bool> {
----------------
Doug Gregor wrote:
> Why not just fold this functionality into the RecursiveASTVisitor?
Ok
================
Comment at: include/clang/AST/RecursiveASTVisitor.h:2280
@@ +2279,3 @@
+ bool Visit##Class(Class *S) { \
+ for (Stmt::child_range Range = S->children(); Range; ++Range) { \
+ if (!Visitor->TraverseStmt(*Range)) return false; \
----------------
Doug Gregor wrote:
> I wonder... rather than propagating the children() anti-pattern from
> Stmt/Expr, should we just implement the various Visit* methods directly in
> the RecursiveOMPClauseVisitor?
Ok, I'll remove children
================
Comment at: include/clang/AST/StmtOpenMP.h:32-35
@@ +31,6 @@
+class OMPClause {
+ /// \brief Starting location of the clause.
+ SourceLocation StartLoc;
+ /// \brief Ending location of the clause.
+ SourceLocation EndLoc;
+ /// \brief Kind of the clause.
----------------
Dmitri Gribenko wrote:
> Please clarify what these locations are. We probably need locations for:
> clause keyword, open paren, close paren.
StartLoc - for clause keyword
EndLoc - last symbol for the clause (close paren or clause keyword, if no
parameters)
I will add LParenLoc for open paren
================
Comment at: include/clang/AST/StmtOpenMP.h:58-60
@@ +57,5 @@
+
+ static bool classof(const OMPClause *T) {
+ return true;
+ }
+
----------------
Dmitri Gribenko wrote:
> I could be wrong, but IIRC this is not actually needed. dyn_cast magic
> should figure this out, I think.
Tried to remove these methods. Required for cast.
================
Comment at: include/clang/AST/StmtOpenMP.h:127
@@ +126,3 @@
+ ///
+ void setCondition(Expr *E) { Condition = cast<Stmt>(E); }
+public:
----------------
Dmitri Gribenko wrote:
> `cast` is not needed here.
Removed
================
Comment at: include/clang/AST/StmtOpenMP.h:136
@@ +135,3 @@
+ OMPIfClause(Expr *E, SourceLocation StartLoc, SourceLocation EndLoc)
+ : OMPClause(OMPC_if, StartLoc, EndLoc), Condition(cast<Stmt>(E)) { }
+
----------------
Dmitri Gribenko wrote:
> `cast<Stmt>` is not needed here. You could effectively replace this with
> `assert(E);`
Removed
================
Comment at: include/clang/AST/StmtOpenMP.h:144
@@ +143,3 @@
+ /// \brief Return condition.
+ Expr *getCondition() { return dyn_cast_or_null<Expr>(Condition); }
+
----------------
Dmitri Gribenko wrote:
> Doug Gregor wrote:
> > Wei Pan wrote:
> > > The constructors assume thatn E is not null. Either change the
> > > constructor or update the get method.
> > More directly, why are we still storing these as Stmt*? They can be Expr*,
> > since we no longer have to have Stmt* to deal with child_iterator baggage.
> Should this member function be const-qualified?
cast is removed.
Expression is stored as Expr *.
Added const-qualified version of method.
================
Comment at: include/clang/AST/StmtOpenMP.h:190
@@ +189,3 @@
+ /// \brief Return number of threads.
+ Expr *getNumThreads() { return dyn_cast_or_null<Expr>(NumThreads); }
+
----------------
Dmitri Gribenko wrote:
> Doug Gregor wrote:
> > Wei Pan wrote:
> > > The same as above.
> > Me too :)
> Also, const-qualified?
Also the answer is still the same. :)
================
Comment at: include/clang/AST/StmtOpenMP.h:213-214
@@ +212,4 @@
+ OpenMPDefaultClauseKind Kind;
+ /// \brief Start location of the kind in cource code.
+ SourceLocation KindLoc;
+
----------------
Dmitri Gribenko wrote:
> Maybe `KindKwLoc`?
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:230
@@ +229,3 @@
+ ///
+ /// \brief A Argument of the clause ('none' or 'shared').
+ /// \brief ALoc Starting location of the argument.
----------------
Wei Pan wrote:
> \param and thhe following \brief's
Agree
================
Comment at: include/clang/AST/StmtOpenMP.h:303-304
@@ +302,4 @@
+ ///
+ static OMPPrivateClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
Dmitri Gribenko wrote:
> This fits in 80 cols.
Ok, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:349
@@ +348,3 @@
+ /// \param C AST context.
+ /// \brief StartLoc Starting location of the clause.
+ /// \brief EndLoc Ending location of the clause.
----------------
Wei Pan wrote:
> \param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:353
@@ +352,3 @@
+ ///
+ static OMPFirstPrivateClause *Create(ASTContext &C,
+ SourceLocation StartLoc,
----------------
Wei Pan wrote:
> Are they all DeclRefExpr? I assume the following code is vaild (gcc 4.6
> compiles), you will generate a MemberExpr not a DeclRefExpr.
>
> void bar(int) { }
> struct C {
> int a;
> void foo() {
> #pragma omp firstprivate(a)
> for (int i = 0; i < 10; i++) {
> bar(a);
> }
> }
> };
>
According to OpenMP standard, 2.11.3.4 firstprivate clause, Restrictions:
A variable that is part of another variable (as an array or structure element)
cannot appear in a firstprivate clause.
In this example 'a' is a structure element, which is not allowed by the
standard. So, the compiler should generate error message.
================
Comment at: include/clang/AST/StmtOpenMP.h:362-363
@@ +361,4 @@
+ ///
+ static OMPFirstPrivateClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
Dmitri Gribenko wrote:
> This fits in 80 cols.
Ok, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:406
@@ +405,3 @@
+ /// \param C AST context.
+ /// \brief StartLoc Starting location of the clause.
+ /// \brief EndLoc Ending location of the clause.
----------------
Wei Pan wrote:
> \param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:419-420
@@ +418,4 @@
+ ///
+ static OMPSharedClause *CreateEmpty(ASTContext &C,
+ unsigned N);
+
----------------
Dmitri Gribenko wrote:
> This fits in 80 cols.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:503
@@ +502,3 @@
+ OpenMPReductionClauseOperator Operator;
+ /// \brief Start location of the kind in cource code.
+ SourceLocation OperatorLoc;
----------------
Dmitri Gribenko wrote:
> Wei Pan wrote:
> > location of the reduction operator kind.
> Actually, it is just 'location of the reduction operator'.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:563-567
@@ +562,7 @@
+
+ /// \brief Fetches operator for the clause.
+ OpenMPReductionClauseOperator getOperator() const { return Operator; }
+
+ /// \brief Fetches location of clause operator.
+ SourceLocation getOperatorLoc() const { return OperatorLoc; }
+
----------------
Dmitri Gribenko wrote:
> Bikeshedding: s/Fetches/Returns/ everywhere?
>
> These are more or less synonyms, but 'returns' is typically used where the
> value being returned is not being computed or fetched from somewhere else.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:581
@@ +580,3 @@
+template <typename T> struct make_const_ptr_clause { typedef const T *type; };
+/// \brief This class implements a simple visitor for OMPClause
+/// subclasses.
----------------
Wei Pan wrote:
> Merge into the previous line.
Will be removed.
================
Comment at: include/clang/AST/StmtOpenMP.h:609
@@ +608,3 @@
+
+template<class ImplClass, typename RetTy=void>
+class OMPClauseVisitor :
----------------
Wei Pan wrote:
> A few extra whitespaces RetTy = void
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:627-630
@@ +626,6 @@
+ OpenMPDirectiveKind Kind;
+ /// \brief Starting location of the directive kind.
+ SourceLocation StartLoc;
+ /// \brief Ending location of the directive.
+ SourceLocation EndLoc;
+ /// \brief The statement associated with the directive.
----------------
Dmitri Gribenko wrote:
> Please clarify where these point to (omp keyword? directive keyword -- like
> parallel? #pragma?)
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:633-636
@@ +632,6 @@
+ Stmt *AssociatedStmt;
+ /// \brief Number of clauses.
+ unsigned NumClauses;
+ /// \brief Pointer to the list of clauses.
+ OMPClause ** const Clauses;
+protected:
----------------
Dmitri Gribenko wrote:
> ArrayRef?
Yes, I'll fix it
================
Comment at: include/clang/AST/StmtOpenMP.h:645
@@ +644,3 @@
+ /// \param N Number of clauses.
+ /// \param ClausesAndStmt A pointer to the buffer for clauses.
+ ///
----------------
Dmitri Gribenko wrote:
> There's no 'ClausesAndStmt' parameter in the function declaration.
>
> Also, ArrayRef?
Yes, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:665
@@ +664,3 @@
+ ///
+ /// \brief Clauses The list of clauses for the directive.
+ ///
----------------
Dmitri Gribenko wrote:
> \param CL
Ok, will be fixed
================
Comment at: include/clang/AST/StmtOpenMP.h:671
@@ +670,3 @@
+ ///
+ /// /param S Associated statement.
+ ///
----------------
Dmitri Gribenko wrote:
> '\param' with backslash -- but you can drop the \param altogether, it is
> obvious from \brief.
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:685
@@ +684,3 @@
+ ///
+ /// \brief Loc New starting location of directive.
+ ///
----------------
Wei Pan wrote:
> \param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:690
@@ +689,3 @@
+ ///
+ /// \brief Loc New ending location of directive.
+ ///
----------------
Wei Pan wrote:
> param
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:701
@@ +700,3 @@
+ ///
+ OMPClause *getClause(unsigned i) {
+ assert(i < NumClauses && "Wrong number of clause!");
----------------
Dmitri Gribenko wrote:
> Do we need the non-const overload at all? It is identical to the const
> overload below.
Ok, will be removed
================
Comment at: include/clang/AST/StmtOpenMP.h:702
@@ +701,3 @@
+ OMPClause *getClause(unsigned i) {
+ assert(i < NumClauses && "Wrong number of clause!");
+ return getClauses()[i];
----------------
Wei Pan wrote:
> index out of bound?
Ok
================
Comment at: include/clang/AST/StmtOpenMP.h:727
@@ +726,3 @@
+
+ static bool classof(const Stmt *S) {
+ return S->getStmtClass() == OMPParallelDirectiveClass;
----------------
Wei Pan wrote:
> move the following two functions to the of the class. This is consistent with
> other statements.
I will change classoff() method for future use.
Method children() is going to be the same for all directives.
================
Comment at: include/clang/AST/StmtOpenMP.h:735
@@ +734,3 @@
+
+ ArrayRef<OMPClause *> clauses() {
+ return getClauses();
----------------
Dmitri Gribenko wrote:
> Drop the non-const overload?
>
> Also, why have this 'alias' for getClauses() anyway? The name is against
> coding standard.
Ok. Method will be renamed.
================
Comment at: include/clang/AST/StmtOpenMP.h:755
@@ +754,3 @@
+ ///
+ /// \param StartLoc Starting location of the directive kind.
+ /// \param EndLoc Ending Location of the directive.
----------------
Wei Pan wrote:
> it is the starting location of the directive not the directive kind?
It is the location of the directive keyword ('parallel', 'for' etc.)
================
Comment at: include/clang/Basic/OpenMPKinds.def:36
@@ +35,3 @@
+
+// OpenMP clause.
+OPENMP_CLAUSE(if, OMPIfClause)
----------------
Wei Pan wrote:
> clauses
Ok
================
Comment at: include/clang/Basic/OpenMPKinds.def:46
@@ +45,3 @@
+
+// Clauses allowed for OpenMP directives
+OPENMP_PARALLEL_CLAUSE(if)
----------------
Dmitri Gribenko wrote:
> '.' at the end.
Ok
================
Comment at: lib/AST/Stmt.cpp:1139
@@ +1138,3 @@
+StmtRange OMPClause::children() {
+ switch(getClauseKind()) {
+ default : break;
----------------
Dmitri Gribenko wrote:
> Space before '('
The method will be removed
================
Comment at: lib/AST/StmtPrinter.cpp:587
@@ +586,3 @@
+namespace {
+class OMPClausePrinter : public OMPClauseVisitor<OMPClausePrinter> {
+ StmtPrinter *Printer;
----------------
Dmitri Gribenko wrote:
> Make sure to add tests for printing when parsing and semantic analysis is
> implemented.
Of course, the tests for printing are developed already.
================
Comment at: lib/AST/StmtPrinter.cpp:665
@@ +664,3 @@
+ OS << (I == Node->varlist_begin() ? '(' : ',')
+ << *cast<NamedDecl>(cast<DeclRefExpr>(*I)->getDecl());
+ }
----------------
Wei Pan wrote:
> This needs to change, if it is a MemberExpr. The same as the others.
MemeberExpr is not allowed per standard
================
Comment at: lib/AST/StmtPrinter.cpp:705
@@ +704,3 @@
+ if (Node->getAssociatedStmt()) {
+ assert(isa<CompoundStmt>(Node->getAssociatedStmt()) &&
+ "Expected compound statement!");
----------------
Wei Pan wrote:
> Cannot be a for statement or AttributedStmt?
>
> #pragma omp firstprivate(a)
> [[some C++11 attributes]] for (int i = 0; i < 10; i++) {
> }
>
All statements are aggregated into CompoundStmt (for jump analysis). So the top
statement should be a CompoundStmt.
================
Comment at: lib/Basic/OpenMPKinds.cpp:53
@@ +52,3 @@
+
+const char *clang::getOpenMPClauseName(OpenMPClauseKind Kind) {
+ assert(Kind < NUM_OPENMP_CLAUSES);
----------------
Doug Gregor wrote:
> This isn't actually used right now. What's it's purpose? It would be fine if
> it's part of debugging, but generally we avoid splicing strings like this
> into diagnostics (for example).
It is used for diagnostic.
For example, if the variable is marked as private, it cannot be shared. So, in
this case a message is generated:
"%0 variable cannot be %1", where %0 and %1 are generated by
getOpenMPClauseName(CurrentVarKind) and getOpenMPClauseName(CurrentClauseKind).
================
Comment at: lib/Basic/OpenMPKinds.cpp:63-64
@@ +62,4 @@
+ return "threadprivate or thread local";
+ default:
+ break;
+ }
----------------
Dmitri Gribenko wrote:
> 'default' should not be needed -- here and in other functions in this file.
There is NUM_OPENMP_CLAUSES, which is not covered. Should I include it
explicitly into the switch?
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1616-1617
@@ +1615,4 @@
+ break;
+ default:
+ return 0;
+ }
----------------
Dmitri Gribenko wrote:
> Is this possible at all?
No, I'll remove it.
================
Comment at: tools/libclang/CIndex.cpp:1920
@@ +1919,3 @@
+ unsigned size = WL.size();
+ for (Stmt::const_child_range Child = S->children(); Child; ++Child) {
+ AddStmt(*Child);
----------------
Wei Pan wrote:
> no {
>
> and
>
> }
Ok
http://llvm-reviews.chandlerc.com/D572
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
