[PATCH] D135594: [ADT] Extend EnumeratedArray

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Generally looks OK - few stylistic-y nits.



================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:46
   }
+  EnumeratedArray(std::initializer_list<ValueType> Init) {
+    assert(Init.size() == Size && "Incorrect initializer size");
----------------
might also want a deduction guide for this?


================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:53
+
   inline const ValueType &operator[](const Enumeration Index) const {
     auto IX = static_cast<const IndexType>(Index);
----------------
No need to use the `inline` keyword for member functions defined in the class body. (it doesn't change the linkagee, though it does hint the compiler towards inlining more - which, without perf investigation, is probably not what we want to do by default)


================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:54
   inline const ValueType &operator[](const Enumeration Index) const {
     auto IX = static_cast<const IndexType>(Index);
     assert(IX >= 0 && IX < Size && "Index is out of bounds.");
----------------
don't need the const here - we don't usually use top level const (& the auto will deduce non-const anyway)


================
Comment at: llvm/unittests/ADT/EnumeratedArrayTest.cpp:51
+
+  EnumeratedArray<float, Colors, Colors::Last, size_t> Array3{10.0, 11.0, 12.0};
+  EXPECT_EQ(Array3[Colors::Red], 10.0);
----------------
generally prefer value initialization (with `=`) rather than direct initialization, then it can't get confused with other ctors - easier to read (as a reader I know it's calling an implicit ctor, which is more likely to be the "simple" thing)


================
Comment at: llvm/unittests/ADT/EnumeratedArrayTest.cpp:65
+
+  const EnumeratedArray<float, Colors, Colors::Last, size_t> Array{};
+
----------------
Don't need the {} here, I think?
(& again, we usually don't bother with top-level const on local variables in LLVM code)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135594