<div dir="auto">Thanks for the comments, however this patch is nowhere near close to ready for review. I was going to make more changes before adding reviewers.</div><div class="gmail_extra"><br><div class="gmail_quote">On Sep 6, 2017 9:27 PM, "Zachary Turner via Phabricator" <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">zturner added a comment.<br>
<br>
Try to be more careful so as not to submit testing code / comments etc in uploaded patches.  Reasons like this are why I prefer using a debugger instead of printf debugging :)<br>
<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/IR/<wbr>DebugInfoMetadata.h:476-481<br>
+  enum class ChecksumKind : uint8_t {<br>
     CSK_None,<br>
     CSK_MD5,<br>
     CSK_SHA1,<br>
     CSK_Last = CSK_SHA1 // Should be last enumeration.<br>
   };<br>
----------------<br>
If you're going to make it an enum class, then it doesn't need the `CSK` prefix in the name, since you have to write `ChecksumKind` anyway.  I wouldn't suggest churning the code to change the names everywhere it's used, so instead maybe just leave this as a regular enum.  It's already scoped to `DIFile` anyway, so there's no risk of namespace pollution.<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/MC/<wbr>MCCodeView.h:297-300<br>
   SmallVector<StringRef, 4> Filenames;<br>
<br>
+  // Map from filename to checksum.<br>
+  StringMap<std::pair<StringRef, uint8_t>> Checksums;<br>
----------------<br>
Any reason this isn't just:<br>
<br>
```<br>
struct FileInfo {<br>
  StringRef Name;<br>
  StringRef Checksum;<br>
  ChecksumKind Kind;<br>
};<br>
<br>
SmallVector<FileInfo> Files;<br>
```<br>
<br>
having a `StringMap` seems unnecessary if we're already iterating a linear container anyway<br>
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/CodeView/<wbr>DebugChecksumsSubsection.cpp:<wbr>53<br>
 Error DebugChecksumsSubsectionRef::<wbr>initialize(BinaryStreamReader Reader) {<br>
+  //errs() << "checksum bytes remaining " << Reader.bytesRemaining() << "\n";<br>
   if (auto EC = Reader.readArray(Checksums, Reader.bytesRemaining()))<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/CodeView/<wbr>DebugStringTableSubsection.<wbr>cpp:32<br>
 Error DebugStringTableSubsectionRef:<wbr>:initialize(BinaryStreamReader &Reader) {<br>
+  errs() << "String table bytes remaining " << Reader.bytesRemaining() << "\n";<br>
   return Reader.readStreamRef(Stream);<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/CodeView/<wbr>DebugStringTableSubsection.<wbr>cpp:38<br>
 DebugStringTableSubsectionRef:<wbr>:getString(uint32_t Offset) const {<br>
+  //errs() << "DebugStringTableSubsectionRef getString\n";<br>
   BinaryStreamReader Reader(Stream);<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/CodeView/<wbr>DebugStringTableSubsection.<wbr>cpp:44<br>
     return std::move(EC);<br>
+  //errs() << "leaving getString\n";<br>
   return Result;<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/lib/MC/MCCodeView.cpp:<wbr>188-200<br>
+    // std::pair<StringRef, uint8_t> Checksum = Checksums[Filename];<br>
+    // if (!Checksum.second) {<br>
+    //   // There is no checksum.<br>
+    //   OS.EmitIntValue(0, 4);<br>
+    //   continue;<br>
+    // }<br>
+    //OS.EmitIntValue(static_cast<<wbr>uint8_t>(Checksum.first.size()<wbr>), 1);<br>
----------------<br>
What is this?  A bunch of code commented out, and then emitting a bunch of magic numbers?<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/<wbr>COFFDumper.cpp:927<br>
     case DebugSubsectionKind::<wbr>StringTable:<br>
+      errs() << "initializing string table\n";<br>
       error(CVStringTable.<wbr>initialize(ST));<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/<wbr>COFFDumper.cpp:929<br>
       error(CVStringTable.<wbr>initialize(ST));<br>
+      errs() << "initialized string table\n";<br>
       break;<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/<wbr>COFFDumper.cpp:1088<br>
       ListScope S(W, "FilenameSegment");<br>
+      //errs() << "about to pring filename with offset " << Entry.NameIndex << "\n";<br>
       printFileNameForOffset("<wbr>Filename", Entry.NameIndex);<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/<wbr>COFFDumper.cpp:1090<br>
       printFileNameForOffset("<wbr>Filename", Entry.NameIndex);<br>
+      //errs() << "printed filename\n";<br>
       uint32_t ColumnIndex = 0;<br>
----------------<br>
Delete<br>
<br>
<br>
<a href="https://reviews.llvm.org/D37157" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D37157</a><br>
<br>
<br>
<br>
</blockquote></div></div>