[PATCH] D135594: [ADT] Extend EnumeratedArray

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:46
   }
+  EnumeratedArray(std::initializer_list<ValueType> Init) {
+    assert(Init.size() == Size && "Incorrect initializer size");
----------------
jsilvanus wrote:
> dblaikie wrote:
> > might also want a deduction guide for this?
> You mean a deduction guide for `EnumeratedArray`? How would we identify the enum type?
Ah, yeah, fair point - can't deduce all the template parameters just from the list of values.


================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:53
+
   inline const ValueType &operator[](const Enumeration Index) const {
     auto IX = static_cast<const IndexType>(Index);
----------------
jsilvanus wrote:
> 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.
ah, right, sorry, had missed that - if you like you could fix the style up in a separate patch (don't need to send a change like that for review - can commit it directly/post-commit review) before this one to simplify the diff here.


================
Comment at: llvm/unittests/ADT/EnumeratedArrayTest.cpp:88-100
+  EXPECT_EQ(*(Array.begin() + 0), 1.0);
+  EXPECT_EQ(*(Array.begin() + 1), 2.0);
+  EXPECT_EQ(*(Array.begin() + 2), 3.0);
+  EXPECT_EQ(*(ConstArray.begin() + 0), 1.0);
+  EXPECT_EQ(*(ConstArray.begin() + 1), 2.0);
+  EXPECT_EQ(*(ConstArray.begin() + 2), 3.0);
+
----------------
I wonder if maybe you can use gmock ElementsAre https://github.com/google/googletest/blob/main/docs/reference/matchers.md to simplify these? (I guess the reverse ones are a bit harder unless you use `llvm::reverse`, which might be more complexity/broader surface area than is desirable - guess you could make_range of rbegin+rend manually, which would probably be OK?)


================
Comment at: llvm/unittests/ADT/EnumeratedArrayTest.cpp:107-122
+TEST(EnumeratedArray, Typedefs) {
+
+  enum class Colors { Red, Blue, Green, Last = Green };
+
+  using Array = EnumeratedArray<float, Colors, Colors::Last, size_t>;
+
+  static_assert(std::is_same<Array::value_type, float>::value,
----------------
Since these are static_asserts, they don't benefit from being in a test function (the test function will do nothing) - maybe pull them out/leave them at the top level?


================
Comment at: llvm/unittests/ADT/EnumeratedArrayTest.cpp:65
+
+  const EnumeratedArray<float, Colors, Colors::Last, size_t> Array{};
+
----------------
jsilvanus wrote:
> 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.
Ah, thanks for explaining - I'm OK either way, just missed/didn't understand. Maybe this change is good though, with the `ConstArray` name making it clear/more self-documenting about why it exists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135594