<div dir="ltr">I like this better I think. A couple of minor points:<br><br>- I'd skip the separate header for buffer_stream and just put it in raw_ostream.h.<br><div><br></div><div>- I'd probably call it raw_pwrite_stream for consistency.</div><div><br></div><div>- I'd suggest a comment around pwrite_stream (by whatever name) that gives some of the context for why you want this. It certainly took me a while to understand the use case: we want to be able to continually stream new bytes into this, and then pwrite over some region.</div><div><br></div><div>From the patch:</div><div><div>+void raw_svector_ostream::pwrite(const char *Ptr, size_t Size,</div><div>+                                 uint64_t Offset) {</div><div>+  flush();</div><div>+</div><div>+  // A pwrite is not supposed to change the file position, but it can change</div><div>+  // the size. In the case of raw_svector_ostream, the size is the position</div><div>+  // so we can only support a pwrite that only overwrites existing data.</div><div>+  uint64_t End = Offset + Size;</div><div>+  assert(End <= OS.size() && "Invalid pwrite");</div></div><div><br></div><div>This seems like a really surprising restriction. Do you see any way to remove it? In particular, it seems like (but I'm likely just missing the point) we could just adjust the position internally to account for the pwrite, and say that the positions clients have captured are invalidated?</div><div><br></div><div>If we really can't fix this, I'd prefer that it were documented on the abstract interface and maybe even checked in more implementations so we don't mess up and code against the more natural pwrite contract when we happen to not have an svector implementation.</div></div><br><div class="gmail_quote">On Fri, Apr 10, 2015 at 3:32 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015-Apr-10, at 15:15, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
><br>
>> Both this and the previous mail have callers of `supportsSeeking()` but no<br>
>> definition (in llvm.patch).  Is the patch out-of-sync somehow?<br>
>><br>
><br>
> No, that was a small addition in r234619 (to raw_fd_ostream only).<br>
<br>
Ah, sorry; my tree was out-of-date.<br>
<br>
FWIW, the most recent one LGTM.  I didn't object to your original<br>
approach, though; I suppose you're waiting to hear from Chandler.</blockquote></div>