[PATCH] D111854: [OPENMP51]Initial parsing/sema for append_args clause for 'declare variant'

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

I'm not super familiar with OpenMP code, but are changes needed for AST serialization as well (so that PCH, modules, etc continue to work properly)?



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1360
+def err_omp_unexpected_append_op : Error<
+  "unexpected append-op in 'append_args' clause, expected 'interop'">;
 def err_omp_declare_variant_wrong_clause : Error<
----------------
I'm hoping this diagnostic makes more sense in situ because I'm not certain what it's telling me.  `append-op` is not a term of art we've used elsewhere, is there a better term we could use or will this be familiar to OpenMP users?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1451
+              << getOpenMPClauseName(CKind) << 0;
+          IsError = true;
+        }
----------------
Do we need to do any error recovery here for the parser to get back to a known-perhaps-good state?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1501
+  while (Tok.is(tok::identifier)) {
+    if (PP.getSpelling(Tok) == "target") {
+      // OpenMP 5.1 [2.15.1, interop Construct, Restrictions]
----------------
Because we already know the token is an identifier, we don't need to go back to the preprocessor for this (and that particular interface involves memory allocations because it uses `std::string`). I think you can do: `Tok.getIdentifierInfo()->isStr("target")` instead.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1544
+  // interop(interop-type[,interop-type]...)
+  while (Tok.is(tok::identifier) && PP.getSpelling(Tok) == "interop") {
+    ConsumeToken();
----------------
Same suggestion here as in the previous function.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1557-1558
+      if (IsTarget && IsTargetSync)
+        InterOpTypes.push_back(
+            OMPDeclareVariantAttr::InteropType::Target_TargetSync);
+      else if (IsTarget)
----------------
Maybe add some comments here about why this is correct and not diagnosed?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1573-1574
+    Diag(Tok.getLocation(), diag::err_omp_unexpected_append_op);
+    SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
+              StopBeforeMatch);
+  }
----------------
Does this need to be a while loop because OpenMP is weird about SkipUntil?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7022-7024
+      FunctionType::ExtInfo Ext = FTy->getExtInfo();
+      FunctionProtoType::ExtProtoInfo EPI;
+      EPI.ExtInfo = Ext;
----------------



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7025-7027
+      QualType RetTy = Context.VoidTy;
+      PTy = dyn_cast<FunctionProtoType>(
+          Context.getFunctionType(RetTy, None, EPI));
----------------
Why is this valid? (Functions without prototypes do specify their return type information btw, so it's very weird that we convert this to return void when ginning up a prototype).


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7029-7030
+    }
+    llvm::SmallVector<QualType, 8> Params;
+    Params.append(PTy->param_type_begin(), PTy->param_type_end());
+
----------------
I'd like an assertion that `PTy` is nonnull.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7036
+                        SR.getBegin(), Sema::LookupOrdinaryName);
+    if (LookupName(Result, getCurScope())) {
+      NamedDecl *ND = Result.getFoundDecl();
----------------
If the lookup fails, silently using a void pointer type is okay?


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

https://reviews.llvm.org/D111854