[PATCH] D30959: [pdb] Add support for writing Module Info and module symbols

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.

LGTM



================
Comment at: llvm/lib/DebugInfo/PDB/Native/ModInfoBuilder.cpp:79
+  Layout.LineBytes = 0;
+  (void)Layout.Mod;         // Set in constructor
+  (void)Layout.ModDiStream; // Set in finalizeMsfLayout
----------------
zturner wrote:
> inglorion wrote:
> > What does the comment here mean? What does this line do?
> I was trying to make it clear that I wasn't forgetting to set any of the fields in this class, but that some of them should already have been set in other places.
But it's a bit puzzling. At first, I thought that Layout.Mod was a volatile variable and reading from the variable is needed here for some reason. Maybe just leaving a comment is better.


================
Comment at: llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp:374
 
-  ExitOnErr(Builder.initialize(YamlObj.Headers->SuperBlock.BlockSize));
+  uint32_t BlockSize = 4096;
+  if (YamlObj.Headers.hasValue())
----------------
It may be nice to leave a comment about 4096. It's just a reasonable default block size for a PDB file and used if a YAML file doesn't specify any block size, right?


https://reviews.llvm.org/D30959