jdoerfert marked 5 inline comments as done. jdoerfert added inline comments.
================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011 unsigned NamedModifiersNumber = 0; - SmallVector<const OMPIfClause *, OMPC_unknown + 1> FoundNameModifiers( - OMPD_unknown + 1); + SmallVector<const OMPIfClause *, unsigned(OMPC_unknown) + 1> + FoundNameModifiers(unsigned(OMPD_unknown) + 1); ---------------- Meinersbur wrote: > ABataev wrote: > > jdoerfert wrote: > > > ABataev wrote: > > > > jdoerfert wrote: > > > > > ABataev wrote: > > > > > > JonChesterfield wrote: > > > > > > > I wonder if it would be worth wrapping the accesses to > > > > > > > FoundNameModifiers in functor that does the enum class to > > > > > > > unsigned conversion. E.g. a class instance that contains the > > > > > > > small vector and exposes operator[] that takes the enum class. > > > > > > > > > > > > > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise. > > > > > > Why `map`? It can be represented as a simple c-style array without > > > > > > any problems. > > > > > No, these are not enums that auto-convert to unsigned. > > > > Convert them manually. Replacing the simple hash array with map does > > > > not look like a good solution. > > > I feel this this is a micro optimization that will make the code less > > > maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is > > > quite a lot of line noise.". > > > Take a look at the first version and let me know if you want to go back > > > to that one for this part, if so, I can do that. > > You already introduced special wrapper class in the same file, maybe you > > can reuse for implicit conversions? > We are introducing an `EnumeratedArray` class in > https://reviews.llvm.org/D68827#change-XphX8PAWFr8V . It should reduce the > need for casting and `OpenMPDirectiveKindExWrapper`. Arguably, the easiest and cleanest way is to use a `map` to *map directives to if clauses*. The wrapper is in a different file but I can write another one for here. EnumeratedArray could probably be used here, it is not in yet though. 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
