[PATCH] D28429: Remove the restriction of ten types on AligedCharArrayUnion

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Hmm - did have some comments, but I think, maybe, all the supported compilers support std::aligned_union now, so switching to that might be an/the right option now? *hopeful*



================
Comment at: include/llvm/Support/AlignOf.h:131
+template <typename... Ts>
+struct AlignedCharArrayUnion
+    : llvm::AlignedCharArray<alignof(llvm::detail::AlignerImpl<Ts...>),
----------------
Leave a non-doxy comment about how std::aligned_union could be used once LLVM's supported compilers support it. (GCC 4.8 is what we'd need, I think - oh, 4.8 is our minimum)


================
Comment at: unittests/ADT/AlignOfTest.cpp:31-34
+struct static_tests {
+  static_assert(sizeof(U) == sizeof(uint64_t[16]), "Statically-computed size must be right");
+  static_assert(alignof(U) == alignof(uint64_t), "Statically-computed alignment must be right");
+};
----------------
These shouldn't need to be in a class like this, should they?


================
Comment at: unittests/ADT/AlignOfTest.cpp:36-41
+TEST(AlignOf, BufferCanBeUsed) {
+  U MyU;
+  memset(MyU.buffer, 'X', sizeof(U));
+  
+  EXPECT_EQ(MyU.buffer[sizeof(uint64_t[16]) - 1], 'X');
+}
----------------
Not sure this test is needed/covers anything - tests that memset works, really? But, yeah... not sure what the right/appropriate testing would be for this.


================
Comment at: unittests/ADT/AlignOfTest.cpp:38
+  U MyU;
+  memset(MyU.buffer, 'X', sizeof(U));
+  
----------------
Should that be sizeof(MyU.buffer) rather than sizeof(U)? (it could have extra padding, etc - I guess not with this specific set of types)


https://reviews.llvm.org/D28429