[PATCH] D28707: PDB: Add a class to create the /names stream contents.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/NameHashTableBuilder.cpp:22
+// a 4-byte hash version and a 4-byte size string table.
+static const uint32_t HeaderSize = 12;
+
----------------
`NameHashTable` has a structure for reading the header into.  I think we should re-use that rather than hardcoding the number.  Can you move the `Header` structure from `NameHashTable.cpp` into the `.h` file, and then re-use it here by filling out the fields and then just calling `Writer.writeObject(H)`?


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/NameHashTableBuilder.cpp:33-34
+
+  // If a given string didn't exist in the string table,
+  // we want to increment the string table size.
+  if (P.second)
----------------
Nitpick: This looks like it's wrapped too soon, well before before 80 characters.


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/NameHashTableBuilder.cpp:40
+
+static uint32_t computeHashEntries(uint32_t NumStrings) {
+  // The /names stream is basically an on-disk open-addressing hash table.
----------------
Can you call this `computeBucketCount`?  compute entries sounds like you might compute the actual values of some entries rather than the number of them.


================
Comment at: llvm/lib/DebugInfo/PDB/Raw/NameHashTableBuilder.cpp:49
+
+std::vector<uint8_t> NameHashTableBuilder::build() const {
+  uint32_t NumEntries = computeHashEntries(Strings.size());
----------------
I would prefer if this method is called `commit()` and takes the same signature as the other methods.  (i.e. a `StreamWriter&`).  You will also need to add a `calculateSerializedLength()` method.  If you want to write to a buffer and return it, you can do something like this:

```
std::vector<uint32_t> Buffer(Builder.calculateSerializedLength());
msf::MutableByteStream Stream(Buffer);
msf::StreamWriter Writer(Stream);
if (auto EC = Builder.commit(Writer))
  return EC;
// The bytes of `Buffer` should be filled out now.
```

This also makes the body of the method cleaner, because it gets rid of a lot of the offset bookkeeping and manual incrementing, and it handles endianness for you as well.

```
if (auto EC = Writer.writeObject(H)) return EC;
for (auto Pair : Strings) {
  Writer.setOffset(Pair.second);
  if (auto EC = Writer.writeFixedString(S))
    return EC;
}

Writer.setOffset(StringSize);
if (auto EC = Writer.writeInteger(NumEntries) return EC;
```

etc etc.


https://reviews.llvm.org/D28707