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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
