[PATCH] D32870: [pdb] Don't build the entire source file list up front.

[Zachary Turner via Phabricator via llvm-commits]

zturner marked 7 inline comments as done.
zturner added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleList.cpp:151
+
+  // At this point, if they are both end iterators, they're not compatible.
+  if (isEnd() && R.isEnd())
----------------
amccarth wrote:
> Huh?  Couldn't they both be the end of the same module?  I don't see where you've ruled that out at this point.
Good catch.  I think this newer logic is much easier to follow.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleList.cpp:198
+  // more than 64k modules (e.g. object files) as well.  So we ignore this
+  // field.
+  if (auto EC = FISR.readArray(ModIndexArray, FileInfoHeader->NumModules))
----------------
amccarth wrote:
> I'm having trouble understanding this comment, for a few reasons:
> 
> 1.  I'm having trouble figuring out exactly what some of the "it"s refer to.
> 2.  It says we're not going to use the first array, but, in fact, you save it into a member variable `ModIndexArray`.  If we're not going to use it, why keep it around?
> 3.  Are you using "modules" to mean more than one thing here?  We're talking about compilands here, right?
> 4.  I don't follow that logic that >64K source files means >64K object files.  Does source files include header files?  If so, isn't it likely that there are fewer object files than source files?
The comment was actually all kinds of wrong, hopefully it's more clear now.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleList.cpp:207
+  for (auto Count : ModFileCountArray)
+    NumSourceFiles += Count;
+
----------------
amccarth wrote:
> But the comment above just said we don't use `NumSourceFiles`, so why sum them up.  I assume that's a comment bug.
It was confusingly referring to `NumSourceFiles` inside of the header.  Fixed.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleList.cpp:216-219
+  // In the reference implementation, this array is where the pointer documented
+  // at the definition of ModuleInfoHeader::FileNameOffs points to.  Note that
+  // although the field in ModuleInfoHeader is ignored this array is not, as it
+  // is the authority on where each filename begins in the names buffer.
----------------
This comment was also wrong btw, so it's also fixed.


https://reviews.llvm.org/D32870