[PATCH] D21011: [codeview] Add complex record type translation

[Reid Kleckner via llvm-commits]

rnk added a comment.

I still feel like this patch is too large and is trying to do too much. Can we limit it to just handling non-virtual methods and their overloads? I don't want to review the VBPtr offset computation code at the same time as bitfields and virtual bases and etc etc. Each of those things requires a certain amount of care.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1029
@@ +1028,3 @@
+
+  // TODO: We should use DW_AT_calling_convention to determine what CC this
+  // procedure record should have.
----------------
You should rebase your patch to pick up the calling convention changes, this TODO should be done.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1132-1137
@@ +1131,8 @@
+  // methodName -> [<DISubprogram, Introduced>]
+  typedef std::map<StringRef,
+                   std::vector<std::pair<const DISubprogram *, bool>>>
+      MethodsMap;
+  // methodName -> [DISubroutineType]
+  typedef std::map<StringRef, std::vector<const DISubroutineType *>>
+      VirtualMethodsMap;
+
----------------
You should be able to key these on MDString* instead of StringRef, because MDStrings are uniqued. That will be more efficient.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1209
@@ +1208,3 @@
+  ClassInfo &InherInfo = collectClassInfo(DDTy);
+
+  for (auto &I : InherInfo.VBClasses)
----------------
All this code below here scares me. It feels like an ad-hoc reimplementation of inheritance logic that should really be in clang. I think it would really simplify things if we ignored virtual base classes for now, and didn't try to emit them just yet. It would make it easier to review this patch.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1323
@@ +1322,3 @@
+  bool HasVirtualFuncTab = false;
+  for (auto &I : Info.Methods) {
+    StringRef MethodName = I.first;
----------------
This patch uses too much auto in for loops. The LLVM guidelines say to use it in "places where the type is already obvious from the context". As a reader, Methods is some other data structure that I don't know much about, so it would help me to have the type here.


http://reviews.llvm.org/D21011