[PATCH] D25356: Define PDBFileBuilder::addStream to add stream.

[Zachary Turner via llvm-commits]

zturner added inline comments.


================
Comment at: lib/DebugInfo/PDB/Raw/DbiStreamBuilder.cpp:246-249
   uint32_t Length = calculateSerializedLength();
   if (auto EC = Msf.setStreamSize(StreamDBI, Length))
     return EC;
   return Error::success();
----------------
ruiu wrote:
> zturner wrote:
> > Here you will need to loop through the various optional debug streams and call `setStreamSize()` on each one.
> I don't think so. We create a new stream like this
> 
>   auto ExpectedIndex = Msf.addStream(Data.size());
> 
> and because the size will never change, we don't need to adjust the size afterwards.
Ahh, I didn't see that before.  I will go back and comment up there.


================
Comment at: lib/DebugInfo/PDB/Raw/DbiStreamBuilder.cpp:49
+                                     ArrayRef<uint8_t> Data) {
+  auto ExpectedIndex = Msf.addStream(Data.size());
+  if (!ExpectedIndex)
----------------
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.

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.


https://reviews.llvm.org/D25356