[PATCH] D20435: [codeview] Adding support for CodeView types

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:812
@@ +811,3 @@
+  //
+  if (Type->getName().count("long")) {
+    switch (Index.getSimpleKind()) {
----------------
This can't be right. You want to transform the index if the name has "long" somewhere in it?
I'd suggest matching against "long INT" explicitly.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:838
@@ +837,3 @@
+
+  return itr->second;  /* second is the CodeView type */
+}
----------------
Please use // for comments.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:849
@@ +848,3 @@
+
+void CodeViewDebug::appendType(CodeViewType* CVType) {
+  TypeTable.push_back(CVType);
----------------
Pointers lean right.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:861-862
@@ +860,4 @@
+
+void CodeViewDebug::mapLLVMTypeToCodeViewType(const DIType  *Type,
+                                              CodeViewType  *CVType) {
+  TypeMap.insert(std::make_pair(Type, CVType));
----------------
Please use clang-format here, you have an abnormal amount of space between the pointee type and the star.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:896
@@ +895,3 @@
+  CodeViewType *CVType;
+  unsigned int Tag;
+
----------------
The LLVM style is just to use `unsigned` instead of `unsigned int`.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:898-900
@@ +897,5 @@
+
+  if (Type == nullptr) {
+    return getVoidType();
+  }
+
----------------
These extra braces are a little jarring.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:902-917
@@ +901,18 @@
+
+  if (CVType = getMappedCodeViewType(Type)) {
+    return CVType;
+  }
+
+  Tag = Type->getTag();
+  switch (Tag) {
+  case dwarf::DW_TAG_base_type:
+    CVType = lowerTypeBasic(cast<DIBasicType>(Type)); break;
+  case dwarf::DW_TAG_subroutine_type:
+    CVType = lowerTypeSubroutine(cast<DISubroutineType>(Type)); break;
+  default:
+    // TODO: Enable this assert once all types are implemented
+    //assert(Tag != Tag && "Unhandled type Tag!");
+    CVType = nullptr;
+    break;
+  }
+
----------------
I'd just do:

```
if (CodeViewType *CVType = getMappedCodeViewType(Type))
  return CVType;

switch (Tag) {
case dwarf::DW_TAG_base_type:
  return lowerTypeBasic(cast<DIBasicType>(Type));
case dwarf::DW_TAG_subroutine_type:
  return lowerTypeSubroutine(cast<DISubroutineType>(Type));
default:
  // TODO: Enable this assert once all types are implemented
  //assert(Tag != Tag && "Unhandled type Tag!");
  return nullptr;
}
```

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:914
@@ +913,3 @@
+    // TODO: Enable this assert once all types are implemented
+    //assert(Tag != Tag && "Unhandled type Tag!");
+    CVType = nullptr;
----------------
Er, `Tag != Tag`?  Perhaps you want `llvm_unreachable` ?

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:956-961
@@ +955,8 @@
+    DITypeRefArray Elements, uint16_t FirstArgIndex) {
+  CodeViewTypeArgumentList *ArgList;
+  DIType                   *ArgType;
+
+  // Create the argument list type entry.
+  //
+  ArgList = CodeViewTypeArgumentList::create();
+
----------------
If there is a single path between a variable and it's initialization, we typically sink the definition to the initializer.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:990-995
@@ +989,8 @@
+CodeViewType *CodeViewDebug::lowerTypeSubroutine(const DISubroutineType *Type) {
+  CodeViewTypeProcedure    *CVType;
+  CodeViewTypeArgumentList *ArgListType;
+  CodeViewType             *ReturnType;
+  int                      CallingConvention;
+  uint16_t                 ParamCount;
+  DITypeRefArray           Arguments;
+
----------------
Ditto.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1134
@@ +1133,3 @@
+
+void CodeViewDebug::emitPadding(unsigned int Count) {
+  static const int PaddingArray[16] = {
----------------
`unsigned`

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1143
@@ +1142,3 @@
+
+  for (unsigned int I = Count; I > 0; --I) {
+    addComment("Padding");
----------------
`unsigned`

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1154-1156
@@ +1153,5 @@
+void CodeViewDebug::emitType(const CodeViewType *CVType) {
+  CodeViewTypeKind Kind;
+
+  Kind = CVType->getTypeKind();
+  switch (Kind) {
----------------
Please make this one line.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1172
@@ +1171,3 @@
+    // TODO: Enable this assert once all types are implemented
+    //assert(Kind != Kind && invalid type kind);
+    break;
----------------
`llvm_unreachable`?

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.h:152-154
@@ -156,1 +151,5 @@
     FileToFilepathMap.clear();
+    for (auto T : TypeTable) {
+      delete T;
+    }
+    TypeTable.clear();
----------------
Could this be `DeleteContainerPointers(TypeTable)` ?


http://reviews.llvm.org/D20435