[PATCH] D53260: [ADT] Fix a bug in DenseSet's initializer_list constructor.

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: include/llvm/ADT/DenseSet.h:71
   DenseSetImpl(std::initializer_list<ValueT> Elems)
-      : DenseSetImpl(Elems.size()) {
+      : DenseSetImpl(NextPowerOf2(Elems.size())) {
     insert(Elems.begin(), Elems.end());
----------------
craig.topper wrote:
> Shouldn’t this be PowerOf2Ceil? NextPowerOf2 will change a number that is already a power of 2.
Yes it should. Thanks!


================
Comment at: unittests/ADT/DenseSetTest.cpp:83-87
+TYPED_TEST(DenseSetTest, InitializerListWithNonPowerOfTwoLength) {
+  // Make sure the initializer list works even when its length is not a power
+  // of two.
+  TypeParam set({1, 2, 3});
+}
----------------
dblaikie wrote:
> "Don't crash" tests always look a bit suspect to me (like "test that this does anything other than crash" is not a particularly strong constraint). Maybe test that it contains the desired contents after such construction, and/or that the capacity or buckets or whatever is the expected power of two?
I only use a "don't crash" test if the other aspects are tested elsewhere. In this case they're covered by the test above, which just happened to have a power-of-two length initializer (or we would have noticed this bug earlier).

I do not suppose there's any harm in doing a sanity check on the content in the non power-of-two case though. I'll add one.


Repository:
  rL LLVM

https://reviews.llvm.org/D53260