[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D10599: [OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.
From:       Alexey Bataev <a.bataev () hotmail ! com>
Date:       2015-07-31 10:40:39
Message-ID: 6cc94f8f885877484c004e3826525abe () localhost ! localdomain
[Download RAW message or body]

ABataev added a comment.

Richard, thanks for the review. I'll try to fix printing of pragmas.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2816
@@ -2818,1 +2815,3 @@
 
+Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclarationWithPragmas(
+    AccessSpecifier &AS, ParsedAttributesWithRange &AccessAttrs,
----------------
rsmith wrote:
> Please can you split this refactoring out into a separate commit? That'll make the \
> functional change here (handling `annot_pragma_openmp`) much easier for future code \
> archaeologists to see.
Ok, done

================
Comment at: lib/Parse/ParseOpenMP.cpp:50-73
@@ -48,26 +49,26 @@
   for (unsigned i = 0; i < llvm::array_lengthof(F); ++i) {
-    if (!Tok.isAnnotation() && DKind == OMPD_unknown) {
+    if (!Tok.isAnnotation() && DKind == OMPD_unknown)
       TokenMatched =
-          (i == 0) &&
-          !P.getPreprocessor().getSpelling(Tok).compare("cancellation");
-    } else {
+          ((i == 0) &&
+           !P.getPreprocessor().getSpelling(Tok).compare("declare")) ||
+          ((i == 1) &&
+           !P.getPreprocessor().getSpelling(Tok).compare("cancellation"));
+    else
       TokenMatched = DKind == F[i][0] && DKind != OMPD_unknown;
-    }
 
     if (TokenMatched) {
       Tok = P.getPreprocessor().LookAhead(0);
       auto TokenIsAnnotation = Tok.isAnnotation();
       auto SDKind =
           TokenIsAnnotation
               ? OMPD_unknown
               : getOpenMPDirectiveKind(P.getPreprocessor().getSpelling(Tok));
 
       if (!TokenIsAnnotation && SDKind == OMPD_unknown) {
         TokenMatched =
-            ((i == 0) &&
+            ((i == 1) &&
              !P.getPreprocessor().getSpelling(Tok).compare("point")) ||
-            ((i == 1) && !P.getPreprocessor().getSpelling(Tok).compare("data"));
-      } else {
+            ((i == 2) && !P.getPreprocessor().getSpelling(Tok).compare("data"));
+      } else
         TokenMatched = SDKind == F[i][1] && SDKind != OMPD_unknown;
 
----------------
rsmith wrote:
> Bracing here is now inconsistent.
Fixed, thanks

================
Comment at: lib/Parse/ParseOpenMP.cpp:150
@@ +149,3 @@
+                                                       TagDecl);
+    else
+      // Here we expect to see some function declaration.
----------------
rsmith wrote:
> Add braces around this multi-line `else` (and the corresponding `if` for \
> consistency).
Done, thanks

================
Comment at: lib/Parse/ParseOpenMP.cpp:154
@@ +153,3 @@
+             (!Ptr || Ptr.get().isNull())) {
+        if (AS == AS_none || TagType == DeclSpec::TST_unspecified) {
+          MaybeParseCXX11Attributes(Attrs);
----------------
rsmith wrote:
> Do you really need both these checks?
Removed the second one and turned it to assert()

================
Comment at: lib/Parse/ParseOpenMP.cpp:171-177
@@ +170,9 @@
+
+    // Append the current token at the end of the new token stream so that it
+    // doesn't get lost.
+    CachedPragmas.push_back(Tok);
+    // Push back tokens for pragma.
+    PP.EnterTokenStream(CachedPragmas.data(), CachedPragmas.size(),
+                        /*DisableMacroExpansion=*/true,
+                        /*OwnsTokens=*/false);
+    // Parse pragma itself.
----------------
rsmith wrote:
> Why are you doing this delayed parsing? You still seem to have no tests that \
> require it.
We need it for future parsing of clauses associated with the 'declare simd' \
construct. Some of the clauses may have references to function arguments. That's why \
I'm using delayed parsing: at first we need to parse a function along with arguments \
and only then we'll parse all the pragmas along with their clauses and references to \
args. Of course currently it is not used, because this patch does not introduce \
parsing of clauses. It will be added later.

================
Comment at: lib/Sema/SemaOpenMP.cpp:2153
@@ +2152,3 @@
+Sema::DeclGroupPtrTy
+Sema::ActOnOpenMPDeclareSimdDirective(ArrayRef<OMPClause *> Clauses,
+                                      Decl *ADecl, SourceLocation StartLoc) {
----------------
rsmith wrote:
> You don't use `Clauses` for anything in here; is that intentional?
It will be used later when we'll add parsing of clauses for this pragma


http://reviews.llvm.org/D10599




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]