[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:       Richard Smith via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2015-08-26 2:04:31
Message-ID: c6d3221b91d2da332bff22ed22ced632 () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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