[PATCH] Microsoft C Record Layout

[Reid Kleckner]

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:144
@@ +143,3 @@
+  Size = CharUnits::Zero();
+  FieldOffsets.erase(FieldOffsets.begin(), FieldOffsets.end());
+
----------------
Probably just .clear().

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:271
@@ +270,3 @@
+
+void MicrosoftRecordLayoutBuilder::WarnIgnoredBitfieldAndAlignment() {
+}
----------------
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.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:252
@@ +251,3 @@
+    if (FieldAlign > Alignment)
+      WarnIgnoredBitfieldAndAlignment();
+    return;
----------------
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.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:261
@@ +260,3 @@
+    if (FieldAlign > Alignment)
+      WarnUnionedBitfieldAndAlignment();
+  } else {
----------------
Similarly, if there's no behavior change, lib/AST shouldn't generally be warning in the first place.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:212
@@ +211,3 @@
+  if (Width > Context.toBits(FieldSize))
+    ErrorBitfieldIsTooWide();
+
----------------
No need, this is done in lib/Sema/SemaDecl.cpp Sema::VerifyBitField().

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2527
@@ -2526,10 +2526,3 @@
   } else {
-    RecordLayoutBuilder Builder(*this, /*EmptySubobjects=*/0);
-    Builder.Layout(D);
-
-    NewEntry =
-      new (*this) ASTRecordLayout(*this, Builder.getSize(), 
-                                  Builder.Alignment,
-                                  Builder.getSize(),
-                                  Builder.FieldOffsets.data(),
-                                  Builder.FieldOffsets.size());
+    if (getTargetInfo().getPointerWidth(0) == 32 &&
+        getTargetInfo().getCXXABI().isMicrosoft() &&
----------------
I'd probably go with getTargetInfo().getTriple().getArch() == Triple::x86.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2528
@@ +2527,3 @@
+    if (getTargetInfo().getPointerWidth(0) == 32 &&
+        getTargetInfo().getCXXABI().isMicrosoft() &&
+        !D->isMsStruct(*this))
----------------
This doesn't really make sense, since we never set -cxx-abi microsoft for plain C tests.  This should probably be checking for Triple::Win32.  That should exclude the Mingw and Cygwin ABIs.



================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2529
@@ +2528,3 @@
+        getTargetInfo().getCXXABI().isMicrosoft() &&
+        !D->isMsStruct(*this))
+      NewEntry = BuildMicrosoftASTRecordLayout(D);
----------------
I thought we discussed doing bitfields in this patch, which is what MsStruct is all about, right?  Do the tests pass if you flip the logic here?

================
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
----------------
Yeah, .c + -cxx-abi microsoft doesn't really make sense.  The triple should be enough.

================
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);
+
----------------
These decls seem unused.

================
Comment at: test/Sema/ms_bitfield_layout.c:244
@@ +243,3 @@
+
+A a;
+B b;
----------------
Can we put the just after their type, or is that problematic?


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