[PATCH] MS RTTI Generation

[Timur Iskhodzhanov]

Please see some of my drive-by nitpicking.

================
Comment at: lib/CodeGen/CGRTTI.cpp:994
@@ +993,3 @@
+
+/// \brief A Helper class for building MS RTTI types.
+struct MSRTTIBuilder {
----------------
I think an overview comment describing basic ideas, concepts and relations would help here?
You may also consider splitting these ~400LOC into a separate file.

================
Comment at: lib/CodeGen/CGRTTI.cpp:995
@@ +994,3 @@
+/// \brief A Helper class for building MS RTTI types.
+struct MSRTTIBuilder {
+  /// \brief A Helper class that stores information about a base class for the
----------------
make it a `class`
http://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords

================
Comment at: lib/CodeGen/CGRTTI.cpp:1020
@@ +1019,3 @@
+  llvm::StructType *getTypeDescriptorType(StringRef Str);
+  bool DeclareCompleteObjectLocator();
+  bool DeclareClassHierarchyDescriptor();
----------------
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
//The (function) name should ... start with a lower case letter//

================
Comment at: lib/CodeGen/CGRTTI.cpp:1055
@@ +1054,3 @@
+// Used by MSRTTIBuilder::BaseClass::BaseClass
+static int countNonVirtualBases(const CXXRecordDecl *RD) {
+  int Count = 0;
----------------
how about

  static unsigned countNonVirtualBases(const CXXRecordDecl *RD) {
    unsigned Count = RD->getNumBases();
    // Comment explaining why whe should do it recursively.
    for (const CXXBaseSpecifier &Base : RD->bases())
      Count += countNonVirtualBases(Base.getType()->getAsCXXRecordDecl());
    return Count;
  }

?

================
Comment at: lib/CodeGen/CGRTTI.cpp:1092
@@ +1091,3 @@
+  // Forward declare the ClassHierarchyDescriptor because of a cycle in the
+  // rtti types.
+  ClassHierarchyDescriptorType =
----------------
s/rtti/RTTI/?

================
Comment at: lib/CodeGen/CGRTTI.cpp:1095
@@ +1094,3 @@
+      llvm::StructType::create(VMContext, "MSRTTIClassHierarchyDescriptor");
+  // Describe the BaseClassDescriptor Type.
+  SmallVector<llvm::Type *, 16> FieldTypes;
----------------
I think adding vertical whitespace between logical blocks would improve the readability

================
Comment at: lib/CodeGen/CGRTTI.cpp:1097
@@ +1096,3 @@
+  SmallVector<llvm::Type *, 16> FieldTypes;
+  FieldTypes.push_back(CGM.Int8PtrTy);
+  FieldTypes.push_back(CGM.IntTy);
----------------
Is there a (high-level?) comment explaining these structures?

================
Comment at: lib/CodeGen/CGRTTI.cpp:1130
@@ +1129,3 @@
+  SmallString<256> TypeDescriptorMangledName, TypeInfoString;
+  llvm::raw_svector_ostream Out(TypeDescriptorMangledName);
+  Mangler.mangleCXXRTTI(Type, Out);
----------------
I remember there was some cool trick to avoid flushing manually -- probably put {} around the stream definition and usage? This also improves the locality of the variables.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1135
@@ +1134,3 @@
+      Module.getNamedGlobal(TypeDescriptorMangledName);
+  if (TypeDescriptor)
+    return llvm::ConstantExpr::getBitCast(TypeDescriptor, CGM.Int8PtrTy);
----------------
  ...
  if (llvm::GlobalVariable *TypeDescriptor = Module.getNamedGlobal(TypeDescriptorMangledName))
    return llvm::ConstantExpr::getBitCast(TypeDescriptor, CGM.Int8PtrTy);
  
  lvm::raw_svector_ostream Out2 = llvm::raw_svector_ostream(TypeInfoString);
  ...

================
Comment at: lib/CodeGen/CGRTTI.cpp:1169
@@ +1168,3 @@
+MSRTTIBuilder::getClassHierarchyDescriptor(const CXXRecordDecl *RD) {
+  this->RD = RD;
+  Bases.clear();
----------------
This kinda reminds me of global variables.
Is it intended that this class is not re-entrant?
Do you think it's reasonable to split it into shared Context + per-class Builders, similar to MicrosoftVFTableContext/VFTableBuilder ?

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:900
@@ -902,1 +899,3 @@
+  for (VPtrInfo *Info : VFPtrs) {
+    llvm::GlobalVariable *VTable = getAddrOfVTable(RD, (Info)->FullOffsetInMDC);
     if (VTable->hasInitializer())
----------------
the extra parenthesis are not needed anymore?

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:907
@@ -905,3 +906,3 @@
     const VTableLayout &VTLayout =
-        VFTContext.getVFTableLayout(RD, (*I)->FullOffsetInMDC);
+      VFTContext.getVFTableLayout(RD, (Info)->FullOffsetInMDC);
     llvm::Constant *Init = CGVT.CreateVTableInitializer(
----------------
ditto

================
Comment at: test/CodeGenCXX/microsoft-abi-rtti.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple=i386-pc-win32 2>/dev/null %s | FileCheck %s
+
----------------
are you sure it should have 755 permissions?

http://reviews.llvm.org/D3833