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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/DbiModuleList.h:89
+  FixedStreamArray<support::little32_t> FileNameOffsets;
+  FixedStreamArray<support::ulittle16_t> ModIndexArray;
+  FixedStreamArray<support::ulittle16_t> ModFileCountArray;
----------------
It seems ModeIndexArray is not needed?  At least, we don't need to keep it around.  Perhaps it should just be a local variable where you read it in order to skip past it.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleList.cpp:40
+  // - They're not *both* end iterators
+  // - They endness is the same.
+  // Thus, they're compatible iterators pointing to a valid file on the same
----------------
s/They/Their/


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleList.cpp:118
+  uint32_t FirstOffset = Modules->ModuleInitialFileIndex[Modi];
+  FirstOffset += Filei;
+  auto ExpectedValue = Modules->getFileName(FirstOffset);
----------------
The naming here seems misleading.  Once you add `Filei`, it's no longer the _first_ offset.  How about:

    uint32_t Offset = Modules->ModuleInitialFileIndex[Modi] + Filei;




================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleList.cpp:135
+  if (Filei >= Modules->getSourceFileCount(Modi))
+    return true;
+  return false;
----------------
If I'm following, it seems `>=` is just defensive and that `==` should be sufficient.  In that case, what if you `assert(Filei <= Modules->getSourceFileCount(Modi))` to help catch bugs that the defensiveness would ordinarily hide?


================
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())
----------------
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.


================
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))
----------------
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?


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


https://reviews.llvm.org/D32870