<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Sep 30, 2016 at 10:32 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">zturner added inline comments.<br>
<br>
<br>
> PDBFileBuilder.h:53<br>
><br>
> -private:<br>
> -  Expected<msf::MSFLayout> finalizeMsfLayout() const;<br>
> +  Error finalizeMsfLayout();<br>
><br>
<br>
I'm not sure if making this public is a good idea.  This leaves open the possibility that someone could mis-use the class.<br>
<br>
I can think of two solutions that would still keep this private:<br>
<br>
1. Changing `commit` so that instead of taking an `msf::WritableStream` it takes a filename.  Then you create the stream in there.<br></blockquote><div><br></div><div>At first I thought that this approach makes things too tightly coupled, but on second, it seems a practical approach. PDBFileBuilder builds a file, so at end we naturally want to write it to a file. I'll update this patch with this approach.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2. Change the implementation of `FileBufferByteStream` so that instead of taking a `FileOutputBuffer` to its constructor, it only takes a filename.  Add a method to that class called `create(uint64_t size)`.  Then the signature of `commit` is changed to take an `msf::FileBufferByteStream` and after internally finalizing the layout, it calls `stream.create(layout.size)`.<br>
<br>
What do you think?<br>
<br>
<a href="https://reviews.llvm.org/D25107" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25107</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>