[PATCH] Non-POD unions with bit-fields causes an assertion failure at clang/lib/AST/RecordLayoutBuilder.cpp:754

[David Majnemer]

I don't think there is much need to run the test for both x86-64 and aarch64, I'd just run the x86-64 one.

I think it's unneccesary to have two different files here, one file for both tests is sufficient.  Just rename one of the `A` classes to `B`.

Please match the existing convention in test/Layout, rename union_regular_bit_field.cpp to include 'itanium' somewhere in there and please use dashes instead of underscores. Perhaps itanium-x86-union-bitfield.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1350-1351
@@ +1349,4 @@
+static uint64_t
+RoundUpFieldSizeToCharAlignment(const uint64_t FieldSize,
+                                const ASTContext &Context) {
+  uint64_t CharAlignment = Context.getTargetInfo().getCharAlign();
----------------
Functions start with a lower case character.

I would just call this `roundUpSizeToCharAlignment`.  This name, while more generic, is less confusing if we chose to use it in other places of record layout.

`const uint64_t` looks abnormal compared to most of clang's code, I would just go with `uint64_t` here.

================
Comment at: test/Layout/union_regular_bit_field.cpp:1-4
@@ +1,5 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple aarch64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+
----------------
These triples are not specific enough.  Make sure you specify the itanium ABI somehow.

================
Comment at: test/Layout/union_regular_bit_field.cpp:13-14
@@ +12,4 @@
+
+// CHECK:      | [sizeof=4, dsize=1, align=4
+// CHECK-NEXT:     |  nvsize=1, nvalign=4]
+
----------------
Please include more `CHECK` lines so that more tests can easily be added to this file.

================
Comment at: test/Layout/union_wide_bit_field.cpp:1-4
@@ +1,5 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple aarch64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+
----------------
Likewise.

http://reviews.llvm.org/D5775