<div dir="ltr">Hmm, I was getting emails about it, so I just figured it was ready for review. I guess it must have triggered a Phabricator rule that caused it to send me emails. Probably best not to upload until it's ready, unless it's very explicit that it's not ready and you're just soliciting initial feedback. But if you're just uploading and not soliciting any feedback, then I don't see much value in uploading to begin with? (I could be missing something, however)</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 6, 2017 at 9:38 PM Eric Beckmann <<a href="mailto:ecbeckmann@google.com">ecbeckmann@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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" target="_blank">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/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/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/DebugChecksumsSubsection.cpp:53<br>
Error DebugChecksumsSubsectionRef::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/DebugStringTableSubsection.cpp:32<br>
Error DebugStringTableSubsectionRef::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/DebugStringTableSubsection.cpp:38<br>
DebugStringTableSubsectionRef::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/DebugStringTableSubsection.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: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<uint8_t>(Checksum.first.size()), 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/COFFDumper.cpp:927<br>
case DebugSubsectionKind::StringTable:<br>
+ errs() << "initializing string table\n";<br>
error(CVStringTable.initialize(ST));<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:929<br>
error(CVStringTable.initialize(ST));<br>
+ errs() << "initialized string table\n";<br>
break;<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1088<br>
ListScope S(W, "FilenameSegment");<br>
+ //errs() << "about to pring filename with offset " << Entry.NameIndex << "\n";<br>
printFileNameForOffset("Filename", Entry.NameIndex);<br>
----------------<br>
Delete<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1090<br>
printFileNameForOffset("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/D37157</a><br>
<br>
<br>
<br>
</blockquote></div></div>
</blockquote></div>