[PATCH] Microsoft C++ Record Layout

[Reid Kleckner]

  Here's some initial comments, I haven't actually gotten through everything.  Travel and other stuff kept this off my plate.


================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:58-60
@@ +57,5 @@
+  MicrosoftRecordLayoutBuilder(const ASTContext &Context) : Context(Context) {}
+  MicrosoftRecordLayoutBuilder(const MicrosoftRecordLayoutBuilder &)
+  LLVM_DELETED_FUNCTION;
+  void operator=(const MicrosoftRecordLayoutBuilder &) LLVM_DELETED_FUNCTION;
+
----------------
The usual pattern is to make these private as well as deleted, so that you get a link error when they are used in C++98.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:64
@@ +63,3 @@
+  void cxxLayout(const CXXRecordDecl *RD);
+  /// \brief Initializes size and alignemnt and honors some flags.
+  void initializeLayout(const RecordDecl *RD);
----------------
"alignment", you may want to hit the other comments with spellcheck.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:89
@@ +88,3 @@
+
+  /// \brief Updates the alignment of the type.  This function doesn't take any properties (such as packedness) into account.  getAdjustedFieldInfo() adjustes for packedness.
+  void updateAlignment(CharUnits NewAlignment) {
----------------
Reflow

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:184
@@ +183,3 @@
+
+  // If we're not on win32 and using ms_struct the field alignment will be wrong
+  // for 64 bit types, so we fix that here.
----------------
We decided not to thread ms_struct through here, so let's kill this dead code.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:1
@@ +1,2 @@
+//=== MicrosoftRecordLayoutBuilder.cpp - Microsoft record layout helpers ---==//
+//
----------------
I'd like to push to keep this in RecordLayoutBuilder.cpp, even though it doesn't share any code.  I have a feeling that over the long term we may be able to factor these together in a way that we can share the bitfield layout code and split the C++ record layout code.  If we split the files, we're raising the difficulty bar for any potential refactoring.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:231-249
@@ +230,21 @@
+
+void MicrosoftRecordLayoutBuilder::layout(const RecordDecl *RD) {
+  initializeLayout(RD);
+  layoutFields(RD);
+  honorDeclspecAlign(RD);
+}
+
+void MicrosoftRecordLayoutBuilder::cxxLayout(const CXXRecordDecl *RD) {
+  initializeLayout(RD);
+  initializeCXXLayout(RD);
+  layoutVFPtr(RD);
+  layoutNonVirtualBases(RD);
+  layoutVBPtr(RD);
+  layoutFields(RD);
+  DataSize = Size;
+  NonVirtualAlignment = Alignment;
+  layoutVirtualBases(RD);
+  finalizeCXXLayout(RD);
+  honorDeclspecAlign(RD);
+}
+
----------------
Nice!

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:322
@@ +321,3 @@
+  // MSVC potentially over-aligns the vf-table pointer by giving it
+  // the max alignment of all the non-virtual data in the class.  The resulting layout is essentially { vftbl, { nvdata } }.
+  // This is completely unnecessary, but we're not here to pass
----------------
Reflow

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:550
@@ +549,3 @@
+    // The space consumed is in an Alignment sized/aligned block and the v-base
+    // is placed at its alignment offset into the chunck, unless its alignment
+    // is less than the size of a pointer, at which it is placed at pointer
----------------
"chunk"

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:620
@@ +619,3 @@
+}
+;
+
----------------
Loose ;

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:644
@@ +643,3 @@
+  // defined in
+  // in a virtual base's vtable, that virtual bases need a vtordisp.  If Here we
+  // collect a list of classes
----------------
Stray "If"?

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:649
@@ +648,3 @@
+  // contain non-virtual
+  // bases that define functions we override also require vtorcisps, this case
+  // is checked explicitly below.
----------------
s/vtorcisps/vtordisps/

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:667-670
@@ +666,6 @@
+      else
+        do {
+          // The method lives in a base; add the overrides to our working set.
+          work.insert(*i);
+        } while (++i != e);
+      // We've finished processing this element, remove it from the working set.
----------------
Can this be work.insert(i, e)?

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:652
@@ +651,3 @@
+  if (RD->hasUserDeclaredConstructor() || RD->hasUserDeclaredDestructor()) {
+    llvm::SmallPtrSet<const CXXMethodDecl *, 8> work;
+    // Seed the working set with our non-destructor virtual methods.
----------------
The convention is to use UpperCase local variables.  Yeah, it's crazy, but it's the norm.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:682
@@ +681,3 @@
+    const CXXRecordDecl *BaseDecl =
+        cast<CXXRecordDecl>(i->getType()->castAs<RecordType>()->getDecl());
+    if (!HasVtordisp.count(BaseDecl) && RequiresVtordisp(HasVtordisp, BaseDecl))
----------------
This can probably be: i->getType()->getAsCXXRecordDecl()

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:604
@@ +603,3 @@
+static bool
+RequiresVtordisp(const llvm::SmallPtrSet<const CXXRecordDecl *, 2> &HasVtordisp,
+                 const CXXRecordDecl *RD) {
----------------
As discussed in person, I think it will simplify things if we add a 'bool RequiresVtorDisp' to ASTRecordLayout::CXXRecordLayoutInfo.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:654
@@ +653,3 @@
+    // Seed the working set with our non-destructor virtual methods.
+    for (clang::CXXRecordDecl::method_iterator i = RD->method_begin(),
+                                               e = RD->method_end();
----------------
We're in using namespace clang.

================
Comment at: lib/AST/MicrosoftRecordLayoutBuilder.cpp:676-677
@@ +675,4 @@
+
+  // Re-check all of our vbases for vtordisp requirements (in case their
+  // non-virtual bases have vtordisp requirements).
+  for (CXXRecordDecl::base_class_const_iterator i = RD->vbases_begin(),
----------------
In person we tried to come up with a cleaner way to do this, but I can't recall what it was.


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