[PATCH] D20711: [codeview] Improve readability of type record assembly

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Looks good to me.
Only few comments below.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:290
@@ +289,3 @@
+    CommentPrefix += '\t';
+    CommentPrefix += Asm->MAI->getCommentString();
+    CommentPrefix += ' ';
----------------
Can you explain what we need the CommentPrefix string?
We are not going to print it after all, right?

================
Comment at: lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp:26
@@ +25,3 @@
+  // file, so pad it out now.
+  int TotalSize = alignTo(Data.size() + 2, 4);
+  assert(TotalSize - 2 <= UINT16_MAX);
----------------
How about defining two "static const" variables? Like this:
static const int Alignment = 4;
static const int SizeFieldLength = 2;

================
Comment at: lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp:31
@@ +30,3 @@
+  memcpy(Mem + 2, Data.data(), Data.size());
+  for (int I = Data.size() + 2; I < TotalSize; ++I)
+    Mem[I] = LF_PAD0 + (TotalSize - I);
----------------
The Padding is created in the wrong order (check how it is emitted in CodeViewDebug.cpp lines 299-300.
This should be like this:

```
for (int I = TotalSize - (Data.size() + 2) - 1; I >= 0; --I)
  Mem[I] = LF_PAD0 + I;
```

================
Comment at: lib/MC/MCAsmStreamer.cpp:707
@@ +706,3 @@
+  // This is binary data. Print it in a grid of hex bytes for readability.
+  size_t Cols = 4;
+  for (size_t I = 0, EI = alignTo(Data.size(), Cols); I < EI; I += Cols) {
----------------
Should not "Cols" be "static const"?


http://reviews.llvm.org/D20711