[PATCH] D64428: Teach `llvm-pdbutil pretty -native` about `-injected-sources`

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

> I'd like to get feedback on the high-level design here, since I'm not familiar with how things are supposed to be done in this code. In particular, does the split between NativeEnumInjectedSources and InjectedSourceStream make sense? Are these names consistent with the rest of the code?

Looks consistent with the existing DIA interface implementations to me, so I'd go with it.

> The injected source _writing_ is done in PDBFileBuilder. Now that InjectedSourceStream exists, should there be a InjectedSourceStreamBuilder that does the building part? (If so, probably in a follow-up)

Sounds like a good follow-up.



================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumInjectedSources.cpp:40
+public:
+  NativeInjectedSource(InjectedSourceStream::const_iterator I,
+                       PDBFile &File, const PDBStringTable &Strings)
----------------
I think this class only uses `I->second` throughout, so would it be simpler to take a reference to the hash table value? I think it's SrcHeaderBlockEntry, right? I think that would be more readable, since we aren't iterating, we shouldn't need an iterator, we're just looking at one source file entry.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumInjectedSources.cpp:50
+    if (!Name) {
+      consumeError(Name.takeError());
+      return "(invalid file name ref)";
----------------
Hm. I guess perhaps we should've allowed error propagation out of these APIs. I guess that's not fixable without an interface change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64428/new/

https://reviews.llvm.org/D64428