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

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/Basic/Attr.td:2074-2078
@@ +2073,7 @@
+  let Documentation = [OMPDeclareSimdDocs];
+  let AdditionalMembers = [{
+  void printPrettyPragma(raw_ostream &OS, const PrintingPolicy &Policy) const {
+    OS << "\n";
+  }
+  }];
+}
----------------
What's this for?

================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:996-999
@@ -995,2 +995,6 @@
   "expected identifier specifying the name of the 'omp critical' directive">;
+def err_omp_single_decl_in_declare_simd : Error<
+  "single declaration is expected with 'declare simd' directive">;
+def err_omp_decl_in_declare_simd : Error<
+  "function declaration is expected with 'declare simd' directive">;
 
----------------
with -> after, in both diagnstics.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7661
@@ -7660,1 +7660,3 @@
+def err_omp_function_expected : Error<
+  "'#pragma omp declare simd' can be applied to functions only">;
 def err_omp_wrong_cancel_region : Error<
----------------
can be applied to functions only -> can only be applied to functions

================
Comment at: lib/AST/DeclPrinter.cpp:214
@@ +213,3 @@
+      default:
+        if (!PrintPragmas)
+          A->printPretty(Out, Policy);
----------------
It looks like we would no longer print attributes with pragma spelling for non-function declarations after this change (but I don't think we have any such pragmas at the moment). I don't want to ask you to write a bunch more code here that you can't test, but can you add a FIXME somewhere in here to add calls to print pragmas in more places?

================
Comment at: lib/Parse/ParseOpenMP.cpp:133
@@ +132,3 @@
+          << getOpenMPDirectiveName(OMPD_declare_simd);
+      SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);
+    }
----------------
You should bail out if you don't actually find your desired token. If you carry on without checking, your `ConsumeToken` call might consume the `tok::eof` token or some similar bad thing.

================
Comment at: lib/Parse/ParseOpenMP.cpp:144-145
@@ +143,4 @@
+      // Here we expect to see some function declaration.
+      while (Tok.isNot(tok::r_brace) && !isEofOrEom() &&
+             (!Ptr || Ptr.get().isNull())) {
+        if (AS == AS_none) {
----------------
I find this loop slightly surprising: the cases where Parse*Declaration return a null `DeclGroupPtrTy` are when they parsed some declaration-like entity (such as a declaration-like pragma) but didn't create some declaration to represent it.

Do you really want to allow such things between your pragma and its associated function? As an extreme case, you'd accept things like this:

  #pragma omp declare simd
  #pragma pack (...)
  ;
  public:
  __if_exists(foo) {
    int n;
  }
  int the_simd_function();

================
Comment at: lib/Parse/ParseOpenMP.cpp:145
@@ +144,3 @@
+      while (Tok.isNot(tok::r_brace) && !isEofOrEom() &&
+             (!Ptr || Ptr.get().isNull())) {
+        if (AS == AS_none) {
----------------
Do you really need to check both `!Ptr` and `Ptr.get().isNull()` here? Generally, the parser shouldn't really be calling `get()` on an `OpaquePtr` -- these pointers are supposed to be opaque to the parser.

================
Comment at: lib/Parse/ParseOpenMP.cpp:160
@@ +159,3 @@
+        Diag(Loc, diag::err_omp_decl_in_declare_simd);
+      return DeclGroupPtrTy();
+    }
----------------
The only way you could get here without being at EOF or EOM would be if you hit an unexpected right-brace. Shouldn't you diagnose that? For instance, it looks like you won't diagnose this case:

  namespace N {
    #pragma omp declare simd
  }

================
Comment at: lib/Parse/ParseOpenMP.cpp:162-168
@@ +161,9 @@
+    }
+    if (!Ptr.get().isSingleDecl())
+      Diag(Loc, diag::err_omp_single_decl_in_declare_simd);
+
+    if (Ptr.get().isSingleDecl())
+      return Actions.ActOnOpenMPDeclareSimdDirective(Ptr.get().getSingleDecl(),
+                                                     Loc);
+    return Ptr;
+  }
----------------
I would suggest that you instead call `ActOnOpenMPDeclareSimdDirective` unconditionally, pass in `Ptr` rather than its contained declaration, and check for a single declaration in `Sema` -- this seems much more of a semantic check than a syntactic one.


http://reviews.llvm.org/D10599