The reason I still kept reset is because without it, subsequent calls to build will generate invalid layouts, which seemed unintuitive and error prone.  I can fix build() to support this, but it's kind of tricky.  Even if someone can create another builder they shouldn't be able to generate a bad layout.<br><br>About initialize(), In earlier revisions this function could fail, so it needed to return an Error.  But I removed the ability for it to fail.  Although now that I think about it, I should probably validate the block size.<br><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 13, 2016 at 3:30 PM Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ruiu added inline comments.<br>
<br>
================<br>
Comment at: include/llvm/DebugInfo/PDB/Raw/MsfBuilder.h:33<br>
@@ +32,3 @@<br>
+                  bool CanGrow = true);<br>
+  void reset();<br>
+<br>
----------------<br>
zturner wrote:<br>
> ruiu wrote:<br>
> > What's the use case of `reset()`? You can create as many instances of this class as you want, so wouldn't it be easier to make it a non-reusable class?<br>
> `build()` calls `reset()` after it's done to invalidate the entire structure so you have to start over.  It would be nice if you could call `build()`, make one or two simple changes, then call `build()` again, but it's a little bit tricky for technical reasons, so for now I'm just having `build()` invalidate it.<br>
I think what I was trying to say is to move the code from `reset` to the ctor if needed and remove the `reset` member function entirely. If a user wants to create two or more MSF headers, they can create multiple instances of MsfBuilder, so `reset` does not seem necessary.<br>
<br>
================<br>
Comment at: lib/DebugInfo/PDB/Raw/MsfBuilder.cpp:26-27<br>
@@ +25,4 @@<br>
+<br>
+void MsfBuilder::initialize(uint32_t PageSize, uint32_t MinPageCount,<br>
+                            bool CanGrow) {<br>
+  assert(!IsInitialized && "MsfBuilder is already initialized!");<br>
----------------<br>
I think this is a bit error-prone. Why don't you merge this with the ctor?<br>
<br>
<br>
<a href="http://reviews.llvm.org/D22308" rel="noreferrer" target="_blank">http://reviews.llvm.org/D22308</a><br>
<br>
<br>
<br>
</blockquote></div>