[PATCH] Fixing Alias-Avoidance Padding for MS-ABI Layout

Richard Smith richard at metafoo.co.uk
Sat Nov 23 11:07:58 PST 2013


  Code changes LGTM (other than minor issues below). Tests seem to be missing from the patch, though.


================
Comment at: lib/AST/RecordLayout.cpp:57-60
@@ -56,3 +56,6 @@
                                  const CXXRecordDecl *BaseSharingVBPtr,
-                                 bool AlignAfterVBases,
+                                 bool alignAfterVBases,
+                                 bool hasZeroSizedMemberOrBase,
+                                 bool leftMostBaseIsZeroSized,
+                                 bool isEmpty,
                                  const BaseOffsetsMapTy& BaseOffsets,
----------------
If you're going to fix case here, please capitalize rather than lowercasing (to move towards the coding style).

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2025
@@ +2024,3 @@
+//   additional padding between two bases is that the first base is zero sized
+//   or has a zero sized element as a member or base or any member or base has
+//   that property and the second base is zero sized or has a zero sized base
----------------
"as a subobject" would seem more accurate than "as a member or base", since we also look at the element type for an array.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2149
@@ -2144,1 +2148,3 @@
+  /// has been laid out.
+  const ASTRecordLayout* PreviousBaseLayout;
   /// \brief Lets us know if we're in 64-bit mode
----------------
" *"

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2370
@@ -2361,1 +2369,3 @@
 
+  if (Layout->leftMostBaseIsZeroSized() && PreviousBaseLayout &&
+      PreviousBaseLayout->hasZeroSizedMemberOrBase())
----------------
A comment on this would be useful.


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



More information about the cfe-commits mailing list