[PATCH] D22308: [pdb] Introduce an MsfBuilder class for laying out PDB files

[Adrian McCarthy via llvm-commits]

amccarth added a subscriber: amccarth.
amccarth added a comment.

Looks pretty good overall.  I have a few suggestions/questions.


================
Comment at: include/llvm/DebugInfo/PDB/Raw/MsfBuilder.h:17
@@ +16,3 @@
+#include "llvm/DebugInfo/PDB/Raw/MsfCommon.h"
+#include "llvm/DebugInfo/PDB/Raw/PDBFile.h"
+
----------------
The inconsistent capitalization of initialisms seems unfortunate.  `PDB` but `Msf`.

================
Comment at: include/llvm/DebugInfo/PDB/Raw/MsfBuilder.h:38
@@ +37,3 @@
+  Error addStream(uint32_t Size, ArrayRef<uint32_t> Blocks);
+  Error addStream(uint32_t Size);
+  Error setStreamSize(uint32_t Idx, uint32_t Size);
----------------
Some comments describing these members would be nice.  I'd expect something like addStream to return something like an index or handle to refer to that new stream.  What's the difference between the two versions (with and without Blocks)?

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:30
@@ +29,3 @@
+
+  // The super block and block map address are not counted
+  BlockSize = PageSize;
----------------
I think you meant "addresses" (plural).

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:108
@@ +107,3 @@
+
+bool MsfBuilder::isPageFree(uint32_t Idx) const { return FreePages[Idx]; }
+
----------------
Are pages and blocks the same thing?  Maybe blocks are ranges of pages.  It seems `Idx` is used for both stream indexes and page/block indexes.

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:143
@@ +142,3 @@
+  // stream to
+  // automatically.
+  uint32_t ReqBlocks = bytesToBlocks(Size, BlockSize);
----------------
I think this comment would be better in the header by the declaration of the method, as it explains the difference between the two `addStreams`, which users of the API care about.

The funny line break also makes me wonder if you left out some words.

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:221
@@ +220,3 @@
+  Layout L;
+  // The super block should was already allocated, just set it.
+  L.SB = Allocator.Allocate<SuperBlock>();
----------------
"should was" ?

================
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:223
@@ +222,3 @@
+  L.SB = Allocator.Allocate<SuperBlock>();
+  ::memcpy(L.SB->MagicBytes, Magic, sizeof(Magic));
+  L.SB->BlockMapAddr = BlockMapAddr;
----------------
Is there a reason why you're specifying (global) ::memcpy rather than std::memcpy?

================
Comment at: lib/DebugInfo/PDB/Raw/MsfCommon.cpp:18
@@ +17,3 @@
+  // Check the magic bytes.
+  if (memcmp(SB.MagicBytes, Magic, sizeof(Magic)) != 0)
+    return make_error<RawError>(raw_error_code::corrupt_file,
----------------
`::memcmp` for consistency with `::memcpy`s you put elsewhere?


http://reviews.llvm.org/D22308