<div dir="ltr">Currently we're not checking anything before calling `addStream`.  It's the first line of `addDbgStream`.  So even if the function would return an error because it already exists, the extra stream would already have been created.  I suppose if you add a check for that it would be fine.  But conceptually `finalize` is the place where modification to the underlying MSF is supposed to happen.  At the very least though we should be checking in `addStream` and returning an error immediately if someone has already added that stream (although that would prevent you from adding more data to the stream later on, for example).</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 10, 2016 at 3:12 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 class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/PDB/Raw/DbiStreamBuilder.cpp:49<br class="gmail_msg">
+                                     ArrayRef<uint8_t> Data) {<br class="gmail_msg">
+  auto ExpectedIndex = Msf.addStream(Data.size());<br class="gmail_msg">
+  if (!ExpectedIndex)<br class="gmail_msg">
----------------<br class="gmail_msg">
zturner wrote:<br class="gmail_msg">
> I think we shouldn't call `addStream` here.  Suppose someone called `addDbgStream` more than once for the same value of `Type`.  Then you would end up with unreferenced streams in the file.  Maybe instead we can just store the `ArrayRef`, overwriting any previous value of the `ArrayRef` that was there before.  Then in `finalizeMsfLayout` we can add all the streams.<br class="gmail_msg">
><br class="gmail_msg">
> This way everything is semantically "atomic".  No modification to the underlying file happens until you call `finalize()`, so you can do whatever you want until then without risk of messing anything up.<br class="gmail_msg">
How about this? I think this is atomic because we check everything that can fail before calling addStream in this function.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D25356" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25356</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div>