[PATCH] D69952: [OPENMP50]Generalize handling of context matching/scoring.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3310
+    VariadicUnsignedArgument<"CtxSelectorSets">,
+    VariadicUnsignedArgument<"CtxSelectors">,
     VariadicStringArgument<"ImplVendors">
----------------
Is it possible to have a `VariadicObject` type here to represent the hierarchy and the connection between scores, selector sets, and selectors in a more explicit way?


================
Comment at: clang/include/clang/Basic/OpenMPKinds.def:226
+// OpenMP context selectors.
+OPENMP_CONTEXT_SELECTOR(vendor)
 
----------------
Can we add this into `llvm/include/llvm/IR/OpenMPKinds.def` from the very beginning?


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:43
+/// Struct to store the context selectors info.
+template <typename T, typename VectorType = llvm::ArrayRef<T>>
+struct OpenMPCtxSelectorData {
----------------
I feel uneasy about using an ArrayRef for storage as it can easily lead to UB when the underlying values go out of scope.


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:52
+                                 VectorType &&Names)
+      : CtxSet(CtxSet), Ctx(Ctx), Names(Names) {}
+  template <typename U>
----------------
This constructor, together with the default vector type (ArrayRef), will most certainly lead to problems when used. Given that Names is an xvalue, it is basically at the end of its lifetime. If the user did not change the ArrayRef to sth that owns the data, Names will contain stale references.


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:56
+                                 OpenMPContextSelectorKind Ctx, const U &Names)
+      : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
+};
----------------
Why do you need a second template version here?


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:57
+      : CtxSet(CtxSet), Ctx(Ctx), Names(Names.begin(), Names.end()) {}
+};
+
----------------
It seems we always associate a scope with this class (even if the type of the scope varies), right? If so we can add it into this class to simplify the interfaces and tie them together in a nicer way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69952