[PATCH] Microsoft C Record Layout

[Warren Hunt]

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:252
@@ +251,3 @@
+    if (FieldAlign > Alignment)
+      WarnIgnoredBitfieldAndAlignment();
+    return;
----------------
Reid Kleckner wrote:
> So, the problem is that we're dropping the field alignment on the floor?  Did we do this previously?  If there's no behavior change, I don't see a reason to add a corner case diagnostic that will need to have a flag to turn it off.
The idea for this diagnostic is that we're emulating MS behavior and the behavior is not necessarily the behavior the user intends.  In this case the diagnostic could recommend __declspec(align()) instead of a 0-sized bitfield.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:271
@@ +270,3 @@
+
+void MicrosoftRecordLayoutBuilder::WarnIgnoredBitfieldAndAlignment() {
+}
----------------
Reid Kleckner wrote:
> Presumably these are TODO? Even if you don't keep this code, to add diagnostics, look for prior art in tools/clang/include/clang/Basic/DiagnosticSemaKind.td and DiagnosticGroups.td.
Yep, these are TODOs, I'll add a note and get around to them when the rest of the patch is looking good or in a later patch.  They're not critical.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:144
@@ +143,3 @@
+  Size = CharUnits::Zero();
+  FieldOffsets.erase(FieldOffsets.begin(), FieldOffsets.end());
+
----------------
Reid Kleckner wrote:
> Probably just .clear().
Yeah, clear is better.  However! Deleting the line is even better!  We only ever call Initialize once and always before layout so the array is always empty.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:212
@@ +211,3 @@
+  if (Width > Context.toBits(FieldSize))
+    ErrorBitfieldIsTooWide();
+
----------------
Reid Kleckner wrote:
> No need, this is done in lib/Sema/SemaDecl.cpp Sema::VerifyBitField().
Thanks for the pointer!  It's not exactly correct because MS-C++ behaves like C not like C++ so VerifyBitField won't trigger correctly but I can re-use the error message.

================
Comment at: test/Sema/ms_bitfield_layout.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-pc-win32 -fdump-record-layouts -cxx-abi microsoft %s 2>&1 \
+// RUN:            | FileCheck %s
----------------
Reid Kleckner wrote:
> Yeah, .c + -cxx-abi microsoft doesn't really make sense.  The triple should be enough.
Yeah, fixing that along with the triple test.

================
Comment at: test/Sema/ms_bitfield_layout.c:6
@@ +5,3 @@
+int printf(const char *fmt, ...);
+void* memset(void* ptr, int value, size_t num);
+
----------------
Reid Kleckner wrote:
> These decls seem unused.
You are correct.  I'll remove them.


http://llvm-reviews.chandlerc.com/D1026