[PATCH] D10599: [OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.

[Richard Smith]

rsmith added a comment.

Please find a better way to handle this attribute in DeclPrinter. Perhaps you could generalize your existing support to cover all attributes with pragma spelling?


================
Comment at: lib/Parse/ParseDeclCXX.cpp:2816
@@ -2818,1 +2815,3 @@
 
+Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclarationWithPragmas(
+    AccessSpecifier &AS, ParsedAttributesWithRange &AccessAttrs,
----------------
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.

================
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;
 
----------------
Bracing here is now inconsistent.

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

================
Comment at: lib/Parse/ParseOpenMP.cpp:154
@@ +153,3 @@
+             (!Ptr || Ptr.get().isNull())) {
+        if (AS == AS_none || TagType == DeclSpec::TST_unspecified) {
+          MaybeParseCXX11Attributes(Attrs);
----------------
Do you really need both these checks?

================
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.
----------------
Why are you doing this delayed parsing? You still seem to have no tests that require it.

================
Comment at: lib/Sema/SemaOpenMP.cpp:2153
@@ +2152,3 @@
+Sema::DeclGroupPtrTy
+Sema::ActOnOpenMPDeclareSimdDirective(ArrayRef<OMPClause *> Clauses,
+                                      Decl *ADecl, SourceLocation StartLoc) {
----------------
You don't use `Clauses` for anything in here; is that intentional?


http://reviews.llvm.org/D10599