[PATCH] D28715: Raise the PDB Hash Map out of the NameMap class

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/LinearIntegerMap.cpp:22
+Error LinearIntegerMap::load(msf::StreamReader &Stream) {
+  const Header *HAH;
+  if (auto EC = Stream.readObject(HAH))
----------------
ruiu wrote:
> What does HAH stand for?
Left over from old code where it was more obvious.  I can fix this.


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/LinearIntegerMap.cpp:32-33
+
+  Keys.resize(HAH->Capacity);
+  Values.resize(HAH->Capacity);
+
----------------
ruiu wrote:
> You are extending these vectors unconditionally. What was the point of `resize(1)` in the ctor?
You can either create one of these and manipulate it in memory then serialize it, or you can load one from a serialized format.  If you create one from scratch, the capacity needs to be at least 1 so that you can write an item into it.  


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/LinearIntegerMap.cpp:67-70
+  for (uint32_t P : Present)
+    LargestP = P;
+  for (uint32_t D : Deleted)
+    LargestD = D;
----------------
ruiu wrote:
> Does this actually find largest numbers?
Yes since it's a bit vector, the values you get from iterating over it are the indices that contain set bits.  It iterates over the indices which contain 1 bits, in ascending order.  So the last value should be the index of the largest set bit.


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/LinearIntegerMap.cpp:188
+uint32_t LinearIntegerMap::maxSize(uint32_t capacity) {
+  return capacity * 2 / 3 + 1;
+}
----------------
ruiu wrote:
> This needs a comment.
Good point.  FWIW this comes from the Microsoft code, where they want to maintain < 2/3 load factor (size / capacity ratio)


https://reviews.llvm.org/D28715