[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.


================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:408
                                         OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
   assert(CKind <= OMPC_unknown);
----------------
ABataev wrote:
> Why assert is removed?
I can add it back, I mean it would look like this now:
  `assert(unsigned(DKind) <= unsigned(OMPD_unknown));`

(I thought that the idea of enum classes is not to need it but we can keep it).


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:51-60
+// Helper to unify the enum class OpenMPDirectiveKind with its extension
+// OpenMPDirectiveKindEx.
+struct OpenMPDirectiveKindExWrapper {
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKind DK) : Value(unsigned(DK)) {}
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKindEx DKE)
+      : Value(unsigned(DKE)) {}
+  operator unsigned() const { return Value; }
----------------
ABataev wrote:
> Why do we need this?
First, I'm open for suggestions on how to do this in a nicer way :)

Why I did it:
The code below uses two enums as if they were unsigned numbers and one of them is now an enum class 
which requires you to do explicit casts. Without this trickery here we would need to sprinkle `unsigned(XXXX)` in the code below, especially the array `F` which I did not want to do.



================
Comment at: llvm/include/llvm/IR/OpenMPConstants.h:40
+/// Return a textual representation of the directive \p D.
+const char *getOpenMPDirectiveName(Directive D);
+
----------------
ABataev wrote:
> 1. Better to return StringRef, I think.
> 2. Do we really need these 2 convert functions here? Are we going to use them in LLVM or just in clang?
1. Can do, I just moved them in this patch.
2) We will reuse them in Flang so they should be moved (and it seems natural to keep these where the enums are defined.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853