[PATCH] D135594: [ADT] Extend EnumeratedArray

[Jannik Silvanus via Phabricator via llvm-commits]

jsilvanus marked 4 inline comments as done.
jsilvanus added a comment.

Thanks for the review! I've implemented all your suggestions, except for the deduction guide, cf. the inline comment.



================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:46
   }
+  EnumeratedArray(std::initializer_list<ValueType> Init) {
+    assert(Init.size() == Size && "Incorrect initializer size");
----------------
dblaikie wrote:
> might also want a deduction guide for this?
You mean a deduction guide for `EnumeratedArray`? How would we identify the enum type?


================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:53
+
   inline const ValueType &operator[](const Enumeration Index) const {
     auto IX = static_cast<const IndexType>(Index);
----------------
dblaikie wrote:
> 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)
Ok, done.
I was trying to stick to the existing style, but I'm fine with cleaning this up.


================
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.");
----------------
dblaikie wrote:
> don't need the const here - we don't usually use top level const (& the auto will deduce non-const anyway)
Ok, done.


================
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);
----------------
dblaikie wrote:
> 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)
Ok, done!


================
Comment at: llvm/unittests/ADT/EnumeratedArrayTest.cpp:65
+
+  const EnumeratedArray<float, Colors, Colors::Last, size_t> Array{};
+
----------------
dblaikie wrote:
> Don't need the {} here, I think?
> (& again, we usually don't bother with top-level const on local variables in LLVM code)
We need a `const` object or reference to test that `size()` and `empty()` can be called on consts. This was not the case before this patch.
With a const object, clang requires the explicit `{}` initializer, because `EnumeratedArray` has no user-provided default constructor.

To avoid all this, I removed the `const` and the `{}`, and instead use a const reference for the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135594