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

[Reid Kleckner via llvm-commits]

rnk marked an inline comment as done.
rnk added a comment.

Thanks!


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:290
@@ +289,3 @@
+    CommentPrefix += '\t';
+    CommentPrefix += Asm->MAI->getCommentString();
+    CommentPrefix += ' ';
----------------
aaboud wrote:
> Can you explain what we need the CommentPrefix string?
> We are not going to print it after all, right?
The comment block is multiple lines, and we only chop off the comment prefix of the first line.

================
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);
----------------
aaboud wrote:
> aaboud wrote:
> > 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;
> > ```
> It seems that LF_PAD0 should not be used in padding so the correct loop will look like this:
> 
> ```
> for (int I = TotalSize - (Data.size() + 2) ; I > 0; --I)
>   Mem[I] = LF_PAD0 + I;
> ```
I'm pretty sure this loop is correct, because it passes the tests in test/DebugInfo/COFF/inlining-padding.ll for this. We never emit LF_PAD0, because we exit the loop before `I` reaches `TotalSize`.

I thought it was easier to reason about a count-up loop than a count-down loop, so I went with this.

================
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) {
----------------
aaboud wrote:
> Should not "Cols" be "static const"?
I could const-ify it, but I don't generally like making readers think about whether or not local variables require static initialization.


http://reviews.llvm.org/D20711