[PATCH] Support MS Non-Virtual Base Placement Rules in CGRecordLayoutBuilder

David Majnemer david.majnemer at gmail.com
Thu Jan 9 10:17:53 PST 2014

Comment at: lib/CodeGen/CGRecordLayoutBuilder.cpp:149
@@ +148,3 @@
+  /// like MSVC.
+  bool MSLayoutNonVirtualBases(const CXXRecordDecl *RD, 
+                               const ASTRecordLayout &Layout);
Method is called `MSLayoutNonVirtualBases` but comment refers to it as `LayoutNonVirtualBases`

Also, the method name implies that it only performs layout for non-virtual bases but the comment explicitly states that it only performs layout for virtual bases.

Comment at: lib/CodeGen/CGRecordLayoutBuilder.cpp:832
@@ -759,1 +831,3 @@
+        return false;
+    } else if (!LayoutNonVirtualBases(RD, Layout))
       return false;
It makes more sense, at least to me, that `LayoutNonVirtualBases` dispatch to `MSLayoutNonVirtualBases` and `ItaniumLayoutNonVirtualBases` instead of having `LayoutNonVirtualBases` implicitly be the Itanium implementation.

Comment at: lib/CodeGen/CGRecordLayoutBuilder.cpp:735
@@ +734,3 @@
+  // Add a vfptr if the layout says to do so.
+  if (Layout.hasOwnVFPtr()) {
Can we have a series of nv-bases followed by **our** `vfptr` followed by a different series of nv-bases?
Can we have a `vfptr` that proceeds a bunch of nv-bases without a leading `vfptr`?

If the answer to both of these are no, could we sink the code that appends the `vfptr` field right above the code that handles the `vbptr`?


More information about the cfe-commits mailing list