[Openmp-commits] [PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1253
+    : Error<"expected '#pragma omp begin declare variant' to match this stray "
+            "`#pragma omp end delcare variant`">;
 def err_omp_declare_target_unexpected_clause: Error<
----------------
We're not extremely consistent about how we phrase these kinds of errors, but I grepped for stray and we have only one message about stray tokens,  but grepped for matching and we have a bunch more. Phrasing this as:

  '#pragma omp end declare variant' with no matching ''#pragma omp begin declare variant'

might be better.


================
Comment at: clang/include/clang/Sema/Sema.h:9727
+
+  /// The declarator \p D defines a function in the socpe \p S which is nested
+  /// in an `omp begin/end declare variant` scope. In this method we create a
----------------
scope


================
Comment at: clang/lib/Sema/SemaDecl.cpp:13552
+  if (LangOpts.OpenMP && isInOpenMPDeclareVariantScope() &&
+      TemplateParameterLists.empty())
+    BaseFD = ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(
----------------
Please add a comment here explaining why the `TemplateParameterLists.empty()` check is here.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396
+  IdentifierInfo &VariangtII = Context.Idents.get(
+      (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str());
+  D.SetIdentifier(&VariangtII, D.getBeginLoc());
----------------
Is there any way in which this name might become visible to users (e.g., in error messages)?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5471
+      // variant expression. We allow this to fail in which case we continue with
+      // the next best variant expression.
+      Sema::TentativeAnalysisScope Trap(*this);
----------------
Please provide an example or otherwise explain why this failure behavior is correct.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487
+
+    VMIs.erase(VMIs.begin() + BestIdx);
+    Exprs.erase(Exprs.begin() + BestIdx);
----------------
Why is the ordering here useful? Don't you collect all of the variant clauses from all of the declarations? Can't there be duplicates? (and does the relative order need always be the same?) Are we effectively supporting here, as an extension, cases where not all of the declarations have the same set of variants declared (it loos like we are because there's no break in the `while (CalleeFnDecl)` loop, but this makes me wonder if that would still have an odd behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75779