[Openmp-commits] [PATCH] D105648: [OpenMP] Support OpenMP 5.1 attributes

[Erich Keane via Phabricator via Openmp-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/Basic/TokenKinds.def:869
 PRAGMA_ANNOTATION(pragma_openmp)
+PRAGMA_ANNOTATION(pragma_openmp_from_attr)
 PRAGMA_ANNOTATION(pragma_openmp_end)
----------------
`pragma_openmp_from_attr`?  Funny since it isn't a pragma...


================
Comment at: clang/lib/Basic/Attributes.cpp:26
+  // machinery that goes through TableGen.
+  if (LangOpts.OpenMP >= 51 && ScopeName == "omp")
+    return Name == "directive" || Name == "sequence";
----------------
Should we diagnose in non-OMP5.1 uses of this?


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4152-4156
+    BalancedDelimiterTracker T(*this, tok::l_paren);
+    if (T.consumeOpen()) {
+      Diag(Tok, diag::err_expected) << tok::l_paren;
+      return;
+    }
----------------
Can this section be pulled outside of these?  Both seem to have it in common.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4211
+    T.consumeClose();
+  }
+}
----------------
Is 'else' an unreachable'?


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4285
+    // created for us by the caller.
+    return true;
+  }
----------------
Another place to make the same comment :D  I wonder if giving a diagnostic on attempting to use omp::directive in a previous version should have either an extension warning or more explicit warning would be a good idea?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:109
   ParsedAttributesWithRange Attrs(AttrFactory);
-  MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
+  MaybeParseCXX11Attributes(Attrs, nullptr,
+                            /*MightBeObjCMessageSend*/ true);
----------------
Nit: This change is unrelated to the patch?  Seems you have a couple of those (including the case changes of params above).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105648/new/

https://reviews.llvm.org/D105648