Enable the use of pwrite in the object writers.

Chandler Carruth chandlerc at google.com
Fri Apr 10 17:57:45 PDT 2015


I like this better I think. A couple of minor points:

- I'd skip the separate header for buffer_stream and just put it in
raw_ostream.h.

- I'd probably call it raw_pwrite_stream for consistency.

- 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.

>From the patch:
+void raw_svector_ostream::pwrite(const char *Ptr, size_t Size,
+                                 uint64_t Offset) {
+  flush();
+
+  // A pwrite is not supposed to change the file position, but it can
change
+  // the size. In the case of raw_svector_ostream, the size is the position
+  // so we can only support a pwrite that only overwrites existing data.
+  uint64_t End = Offset + Size;
+  assert(End <= OS.size() && "Invalid pwrite");

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?

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.

On Fri, Apr 10, 2015 at 3:32 PM Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Apr-10, at 15:15, Rafael EspĂ­ndola <rafael.espindola at gmail.com>
> wrote:
> >
> >> Both this and the previous mail have callers of `supportsSeeking()` but
> no
> >> definition (in llvm.patch).  Is the patch out-of-sync somehow?
> >>
> >
> > No, that was a small addition in r234619 (to raw_fd_ostream only).
>
> Ah, sorry; my tree was out-of-date.
>
> FWIW, the most recent one LGTM.  I didn't object to your original
> approach, though; I suppose you're waiting to hear from Chandler.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150411/f597562a/attachment.html>


More information about the llvm-commits mailing list