================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
