On Dec 21, 2009, at 8:05 AM, David Greene wrote:> On Friday 18 December 2009 20:20, David Greene wrote: > >> I wanted something to signify in the output where the debug dump starts. >> That's for that case where some stuff might still get output through >> errs(). I anticipate that while I'll change most uses of errs() to dbgs(), >> we probably don't want to change all of them. >> >> I agree the naming here is poor. I'll fix that. > > Here's the next iteration.Gentle reminder, please include patches as attachments, it makes them easier to review.>> Now that I understand the model :) I don't see why you need to fiddle >> with the underlying buffering of TheStream. In the dbgs() use case, >> the underlying stream will be errs() which is already unbuffered. > > Might circular_raw_ostream be used with other streams at some point?In that case, setting the underlying stream to unbuffered seems even more dangerous. The client who sets up the buffers has both the power and the knowledge to set the streams up correctly when the streams are created.> + /// flushBuffer - Dump the contents of the buffer to Stream. > + /// > + void flushBuffer(void) { > + // Write the older portion of the buffer. > + TheStream->write(Cur, BufferArray + BufferSize - Cur); > + // Write the newer portion of the buffer. > + TheStream->write(BufferArray, Cur - BufferArray); > + Cur = BufferArray; > + }Is this correct in the case when the circular buffer hasn't completely filled yet?> +void circular_raw_ostream::flushBufferWithBanner(void) { > + if (BufferSize != 0) { > + // Write out the buffer > + const char *msg = "*** Log Output ***\n";This is not generic. If you really want something like this, the string to print should be provided to the ctor. -Chris
On Monday 21 December 2009 13:40, Chris Lattner wrote:> >> Now that I understand the model :) I don't see why you need to fiddle > >> with the underlying buffering of TheStream. In the dbgs() use case, > >> the underlying stream will be errs() which is already unbuffered. > > > > Might circular_raw_ostream be used with other streams at some point? > > In that case, setting the underlying stream to unbuffered seems even more > dangerous. The client who sets up the buffers has both the power and the > knowledge to set the streams up correctly when the streams are created.Fair enough. I'll make the change. I was simply following what we did for formatted_raw_ostream. It seems a bit wasteful to do buffering on both streams.> > + /// flushBuffer - Dump the contents of the buffer to Stream. > > + /// > > + void flushBuffer(void) { > > + // Write the older portion of the buffer. > > + TheStream->write(Cur, BufferArray + BufferSize - Cur); > > + // Write the newer portion of the buffer. > > + TheStream->write(BufferArray, Cur - BufferArray); > > + Cur = BufferArray; > > + } > > Is this correct in the case when the circular buffer hasn't completely > filled yet?Hmm...Depends on what's "correct." :) You'll get a bunch of garbage output but I don't think it should fault or anything. I don't think there's a way of knowing whether the buffer got filled all the way to the end unless we add another member variable. In practice, it will almost always be filled.> > +void circular_raw_ostream::flushBufferWithBanner(void) { > > + if (BufferSize != 0) { > > + // Write out the buffer > > + const char *msg = "*** Log Output ***\n"; > > This is not generic. If you really want something like this, the string to > print should be provided to the ctor.Ok. Attached an updated patch. -Dave -------------- next part -------------- A non-text attachment was scrubbed... Name: circular_raw_ostream.patch Type: text/x-diff Size: 6986 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091221/3d60c638/attachment.patch>
On Monday 21 December 2009 14:20, David Greene wrote:> Attached an updated patch.Ping? -Dave
On Dec 21, 2009, at 12:20 PM, David Greene wrote:>>> + /// flushBuffer - Dump the contents of the buffer to Stream. >>> + /// >>> + void flushBuffer(void) { >>> + // Write the older portion of the buffer. >>> + TheStream->write(Cur, BufferArray + BufferSize - Cur); >>> + // Write the newer portion of the buffer. >>> + TheStream->write(BufferArray, Cur - BufferArray); >>> + Cur = BufferArray; >>> + } >> >> Is this correct in the case when the circular buffer hasn't >> completely >> filled yet? > > Hmm...Depends on what's "correct." :) You'll get a bunch of garbage > output but I don't think it should fault or anything. I don't think > there's a way of knowing whether the buffer got filled all the way to > the end unless we add another member variable. In practice, it will > almost always be filled.Correct is not emitting garbage. Adding another member is worth it. Saving 4 bytes per circular_raw_ostream doesn't matter when you have a multi kilobyte buffer it is managing. Aside from that, your patch looks fine. Please fix this and commit it, -Chris