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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:23
 /// OpenMP directives.
-enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
-  OMPD_##Name,
-#define OPENMP_DIRECTIVE_EXT(Name, Str) \
-  OMPD_##Name,
-#include "clang/Basic/OpenMPKinds.def"
-  OMPD_unknown
-};
+using OpenMPDirectiveKind = llvm::omp::Directive;
 
----------------
I am not a fan of `using` in header files in general contexts. It's mostly a stylistic preference. Why not use `llvm::omp::Directive` everywhere? It is not much longer than the substitution...


================
Comment at: clang/lib/Basic/OpenMPKinds.cpp:384
                                         OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
+  assert(unsigned(DKind) <= unsigned(OMPD_unknown));
   assert(CKind <= OMPC_unknown);
----------------
I really wish we would not have to add all these casts. They make the code ugly. I prefer `enum class` over `enum`, but if it results in this because most of the code uses enums, maybe is worth using just `enum`.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:6625
 void OMPClauseWriter::VisitOMPClauseWithPreInit(OMPClauseWithPreInit *C) {
-  Record.push_back(C->getCaptureRegion());
+  Record.push_back(uint64_t(C->getCaptureRegion()));
   Record.AddStmt(C->getPreInitStmt());
----------------
hum... is this really necessary? Why not make the type of the elements of `Record` the same as the type returned by `C->getCaptureregion()`?


================
Comment at: llvm/include/llvm/Frontend/OpenMPConstants.h:29-34
+/// Make the enum values available in the llvm::omp namespace. This allows us to
+/// write something like OMPD_parallel if we have a `using namespace omp`. At
+/// the same time we do not loose the strong type guarantees of the enum class,
+/// that is we cannot pass an unsigned as Directive without an explicit cast.
+#define OMP_DIRECTIVE(Enum, ...) constexpr auto Enum = omp::Directive::Enum;
+#include "llvm/Frontend/OpenMPKinds.def"
----------------
Is this really necessary? What's wrong with writing `Directive::OMPD_parallel` when `using namespace omp`?

Also, isn't the information provided by `OMPD` a duplicate of `omp::Directive`? In my mind the acronym expands to **O**pen**MP** **D**irective...

Isn't the following more concise?

```
omp::Directive::parallel
omp::Directive::declare_simd
omp::Directive::whatever
```

IMHO, less is better :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853