[PATCH] D20924: [codeview] Add basic record type translation

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Few more comments.
Reid, I can help you improve the record handling, so let's just solve the easy and urgent issues.
I will upload a followup patch that show you how we can get most of the information about records from current metadata.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1025
@@ +1024,3 @@
+  // Construct the field list and complete type record.
+  TypeRecordKind Kind = getRecordKind(Ty);
+  // FIXME: Other ClassOptions, like ContainsNestedClass and NestedClass.
----------------
At this point you better check if the Ty->isForwardDecl(), if it is the case we should not create a complete class.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1127
@@ -977,4 +1126,3 @@
 
-  auto InsertResult = TypeIndices.insert({Ty, TI});
-  (void)InsertResult;
-  assert(InsertResult.second && "DIType lowered twice");
+  // Don't reuse the hashtable lookup result from above as type lowering can
+  // insert new map entries.
----------------
This comment is confusing me, I do not understand why it is needed!
We cannot use the hashtable lookup, because if we get here we then: I == TypeIndices.emd()
You cannot use I anyway, right?

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1159
@@ +1158,3 @@
+      CompleteTypeIndices.insert({cast<DICompositeType>(Ty), TypeIndex()});
+  if (!InsertResult.second)
+    return InsertResult.first->second;
----------------
I think this check is too later, we need to check that we did not create this complete record before the switch, i.e. at line 1140.


http://reviews.llvm.org/D20924