[PATCH] Microsoft C++ Record Layout

[Reid Kleckner]

  Bunch of nits, basically.  The actual implementation doesn't look like it changed and it looks pretty good.

  With a patch this size, you're going to make sure it doesn't introduce any new warnings during a clang self-host.  You can easily self-host on Linux, or you can try to use mingw on Windows.


================
Comment at: include/clang/AST/RecordLayout.h:244
@@ -228,1 +243,3 @@
 
+  /// hasVFPtr - Does this class have a virtual funciton table pointer.
+  bool hasVFPtr() const {
----------------
"funciton" is the best typo :)

================
Comment at: include/clang/AST/RecordLayout.h:250
@@ +249,3 @@
+  
+  /// hasOwnVBPtr - Does this class provide its own virtual-function
+  /// table pointer, rather than inheriting one from a primary base
----------------
virtual-base, not virtual-function

================
Comment at: include/clang/AST/RecordLayout.h:252
@@ +251,3 @@
+  /// table pointer, rather than inheriting one from a primary base
+  /// class?  If so, it is at offset zero.
+  ///
----------------
I'd drop the "it's at offset zero" comment, since it's possibly buried deep within the primary base.

================
Comment at: include/clang/AST/RecordLayout.h:262
@@ +261,3 @@
+
+  /// hasVBPtr - Does this class have a virtual funciton table pointer.
+  bool hasVBPtr() const {
----------------
"funciton"

================
Comment at: include/clang/AST/RecordLayout.h:244
@@ -228,1 +243,3 @@
 
+  /// hasVFPtr - Does this class have a virtual funciton table pointer.
+  bool hasVFPtr() const {
----------------
Reid Kleckner wrote:
> "funciton" is the best typo :)
I know next to nothing about Doxygen, but the prevailing wisdom today is to avoid repeating the function name.  You can use \brief if it's brief, I think.

================
Comment at: include/clang/AST/RecordLayout.h:268
@@ +267,3 @@
+
+  /// forceAlign - 
+  bool getAlignAfterVBases() const {
----------------
drop

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2351
@@ +2350,3 @@
+  return (D->getASTContext().getTargetInfo().getCXXABI().isMicrosoft() ||
+      D->getASTContext().getTargetInfo().getTriple().getOS() ==
+      llvm::Triple::Win32) &&
----------------
clang-format this

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2353
@@ +2352,3 @@
+      llvm::Triple::Win32) &&
+      D->getASTContext().getTargetInfo().getPointerWidth(0) == 32;
+}
----------------
This probably deserves a FIXME comment explaining that we intend to migrate x64 over, right?

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2389-2396
@@ +2388,10 @@
+
+#include "clang/AST/RecordLayout.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/TargetInfo.h"
+#include "llvm/ADT/SmallSet.h"
+
----------------
You don't need these, do you?

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2356
@@ +2355,3 @@
+
+// This file contains an implementation of struct layout that is, up to the
+// included tests, compatible with cl.exe (2012).  The layouts produced are
----------------
It's not "This file" anymore, it's the MicrosoftRecordLayoutBuilder class.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2398
@@ +2397,3 @@
+
+using namespace clang;
+
----------------
not needed here

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:3081
@@ -2364,3 +3080,3 @@
 
-  const ASTRecordLayout *NewEntry;
+  const ASTRecordLayout *NewEntry = nullptr;
 
----------------
nullptr is C++11, but clang/llvm is C++98.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:3085
@@ -2412,1 +3084,3 @@
+    NewEntry = BuildMicrosoftASTRecordLayout(D);
   } else {
+    if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) {
----------------
I think you can shrink the diff drastically by making this:
  } else if (... dyn_cast<CXXRecordDecl>(...)) {
   ....
  } else {
   ...
  }


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