On Friday 18 December 2009 12:28, Chris Lattner wrote:> This is looking a lot better, here are some more comments: > > + /// current_pos - Return the current position within the stream, > > + /// not counting the bytes currently in the buffer. > > + virtual uint64_t current_pos() { > > I didn't notice this earlier, but is there any reason for current_pos to be > non-const in raw_ostream?Probably not. I'm going to let someone else fix that, though.> > + /// circular_raw_ostream - Open the specified file for > > + /// writing. If an error occurs, information about the error is > > + /// put into ErrorInfo, and the stream should be immediately > > + /// destroyed; the string will be empty if no error occurred. > > This comment is out of date, there is no ErrorInfo.Will remove.> > + /// > > + /// As a side effect, the given Stream is set to be Unbuffered. > > + /// This is because circular_raw_ostream does its own buffering, > > + /// so it doesn't want another layer of buffering to be happening > > + /// underneath it. > > + /// > > + circular_raw_ostream(raw_ostream &Stream, unsigned BuffSize = 8192, > > + bool Delete = false) > > + : raw_ostream(/*unbuffered*/true), > > + TheStream(0), > > + DeleteStream(PRESERVE_STREAM), > > + BufferSize(BuffSize), > > + BufferArray(0) { > > + if (BufferSize > 0) > > + BufferArray = new char[BufferSize]; > > How about just asserting that BufferSize is not zero? This stream won't > work well and doesn't make sense with a buffer of zero.Sure it does. In fact it's crucial for making dbgs() work like errs() in the default case.> > + void setStream(raw_ostream &Stream, bool Delete = false) { > > + releaseStream(); > > + > > + TheStream = &Stream; > > + DeleteStream = Delete; > > + > > + if (BufferSize > 0) { > > The only time where it makes sense for BufferSize to be zero here is if the > circular stream was default constructed. In this case, it seems reasonable > to allocate a buffer here instead of trying to operate without one.See above.> > + void releaseStream() { > > + // Delete the stream if needed. Otherwise, transfer the buffer > > + // settings from this raw_ostream back to the underlying stream. > > + if (!TheStream) > > + return; > > + if (DeleteStream) > > + delete TheStream; > > + else if (BufferSize > 0) > > + TheStream->SetBufferSize(BufferSize); > > + else > > + TheStream->SetUnbuffered(); > > Again, I don't think an unbuffered circular stream makes a lot of sense > here.See above. :)> Another issue is that this is transfering the circular stream's > buffer to the underlying stream. Would it make more sense and would it be > worth it to save the underlying streams buffer size and restore it in > releaseStream?Oh, I think this code is just wrong. I'll rework it.> > +void circular_raw_ostream::write_impl(const char *Ptr, size_t Size) { > > + if (BufferSize > 0) { > > This check can go away.Nope. We don't want to pring the "Log Output" banner if we're not buffering (and delaying output until termination). Again, we want dbgs() to work like errs() in the default case and output immediately.> > + // Write into the buffer, wrapping if necessary. > > + while (Size > 0) { > > + while (Size > 0 && Cur != BufferArray + BufferSize) { > > + --Size; > > + *Cur++ = *Ptr++; > > + } > > This should use memcpy.Ok.> > +void circular_raw_ostream::dumpLog(void) { > > + if (BufferSize > 0) { > > + // Write out the buffer > > + const char *msg = "*** Log Output ***\n"; > > + int num = std::strlen(msg); > > + > > + TheStream->write(msg, num); > > + printLog(); > > + } > > +} > > Is this to debug the implementation of the stream? Should this go into the > tree?No, this is what gets dumped at program termination. Remember, this stream buffers everything you send it, effectively dropping everything except the last N characters, when N == BufferSize. Then at program termination it dumps the buffer. logs() might be a better name than dbgs() for this. What do you think? -Dave
On Friday 18 December 2009 13:53, David Greene wrote:> > > + void releaseStream() { > > > + // Delete the stream if needed. Otherwise, transfer the buffer > > > + // settings from this raw_ostream back to the underlying stream. > > > + if (!TheStream) > > > + return; > > > + if (DeleteStream) > > > + delete TheStream; > > > + else if (BufferSize > 0) > > > + TheStream->SetBufferSize(BufferSize); > > > + else > > > + TheStream->SetUnbuffered(); > > Another issue is that this is transfering the circular stream's > > buffer to the underlying stream. Would it make more sense and would it > > be worth it to save the underlying streams buffer size and restore it in > > releaseStream? > > Oh, I think this code is just wrong. I'll rework it.No, it's right. It's not transferring the buffer at all. It's just setting the buffer size of the held stream when we release it. This code is identical to the code in formatted_raw_ostream. -Dave
On Friday 18 December 2009 13:56, David Greene wrote:> On Friday 18 December 2009 13:53, David Greene wrote: > > > > + void releaseStream() { > > > > + // Delete the stream if needed. Otherwise, transfer the buffer > > > > + // settings from this raw_ostream back to the underlying > > > > stream. + if (!TheStream) > > > > + return; > > > > + if (DeleteStream) > > > > + delete TheStream; > > > > + else if (BufferSize > 0) > > > > + TheStream->SetBufferSize(BufferSize); > > > > + else > > > > + TheStream->SetUnbuffered(); > > > > > > Another issue is that this is transfering the circular stream's > > > buffer to the underlying stream. Would it make more sense and would it > > > be worth it to save the underlying streams buffer size and restore it > > > in releaseStream? > > > > Oh, I think this code is just wrong. I'll rework it. > > No, it's right. It's not transferring the buffer at all. It's just > setting the buffer size of the held stream when we release it. This code > is identical to the code in formatted_raw_ostream.Here's the updated patch. -Dave + /// DELETE_STREAM - Tell the destructor to delete the held stream. + /// + static const bool DELETE_STREAM = true; + + /// PRESERVE_STREAM - Tell the destructor to not delete the held + /// stream. + /// + static const bool PRESERVE_STREAM = false; + + private: + /// TheStream - The real stream we output to. We set it to be + /// unbuffered, since we're already doing our own buffering. + /// + raw_ostream *TheStream; + + /// DeleteStream - Do we need to delete TheStream in the + /// destructor? + /// + bool DeleteStream; + + /// BufferSize - The size of the buffer in bytes. + /// + size_t BufferSize; + + /// BufferArray - The actual buffer storage. + /// + char *BufferArray; + + /// Cur - Pointer to the current output point in BufferArray. + /// + char *Cur; + + /// printLog - Dump the contents of the buffer to Stream. + /// + void printLog(void) { + TheStream->write(BufferArray, BufferSize); + Cur = BufferArray; + } + + virtual void write_impl(const char *Ptr, size_t Size); + + /// current_pos - Return the current position within the stream, + /// not counting the bytes currently in the buffer. + virtual uint64_t current_pos() { + // This has the same effect as calling TheStream.current_pos(), + // but that interface is private. + return TheStream->tell() - TheStream->GetNumBytesInBuffer(); + } + + public: + /// circular_raw_ostream - Open the specified file for + /// writing. + /// + /// As a side effect, if BuffSize is nonzero, the given Stream is + /// set to be Unbuffered. This is because circular_raw_ostream + /// does its own buffering, so it doesn't want another layer of + /// buffering to be happening underneath it. + /// + circular_raw_ostream(raw_ostream &Stream, size_t BuffSize = 8192, + bool Delete = false) + : raw_ostream(/*unbuffered*/true), + TheStream(0), + DeleteStream(PRESERVE_STREAM), + BufferSize(BuffSize), + BufferArray(0) { + if (BufferSize > 0) + BufferArray = new char[BufferSize]; + Cur = BufferArray; + setStream(Stream, Delete); + } + explicit circular_raw_ostream() + : raw_ostream(/*unbuffered*/true), + TheStream(0), + DeleteStream(PRESERVE_STREAM), + BufferArray(0) { + Cur = BufferArray; + } + + ~circular_raw_ostream() { + flush(); + dumpLog(); + releaseStream(); + delete[] BufferArray; + } + + void setStream(raw_ostream &Stream, bool Delete = false) { + releaseStream(); + + TheStream = &Stream; + DeleteStream = Delete; + + if (BufferSize > 0) { + // This circular_raw_ostream will do its own buffering and it + // doesn't need or want TheStream to do another layer of + // buffering underneath. Tell TheStream not to do its own + // buffering. + TheStream->SetUnbuffered(); + } + } + + /// dumpLog - Force output of the buffer along with a small + /// header. + /// + void dumpLog(void); + + private: + void releaseStream() { + // Delete the stream if needed. Otherwise, transfer the buffer + // settings from this raw_ostream back to the underlying stream. + if (!TheStream) + return; + if (DeleteStream) + delete TheStream; + else if (BufferSize > 0) + TheStream->SetBufferSize(BufferSize); + else + TheStream->SetUnbuffered(); + } + }; +} // end llvm namespace + + +#endif Index: lib/Support/circular_raw_ostream.cpp ==================================================================--- lib/Support/circular_raw_ostream.cpp (revision 0) +++ lib/Support/circular_raw_ostream.cpp (revision 0) @@ -0,0 +1,46 @@ +//===- circulat_raw_ostream.cpp - Implement the circular_raw_ostream class -===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This implements support for circular buffered streams. +// +//===----------------------------------------------------------------------===// + +#include "llvm/Support/circular_raw_ostream.h" + +#include <algorithm> + +using namespace llvm; + +void circular_raw_ostream::write_impl(const char *Ptr, size_t Size) { + if (BufferSize > 0) { + // Write into the buffer, wrapping if necessary. + while (Size > 0) { + unsigned Bytes = std::min(Size, BufferSize); + memcpy(Cur, Ptr, Bytes); + Size -= Bytes; + if (Cur == BufferArray + BufferSize) + // Reset the output pointer to the start of the buffer. + Cur = BufferArray; + } + } + else { + TheStream->write(Ptr, Size); + } +} + +void circular_raw_ostream::dumpLog(void) { + if (BufferSize > 0) { + // Write out the buffer + const char *msg = "*** Log Output ***\n"; + int num = std::strlen(msg); + + TheStream->write(msg, num); + printLog(); + } +}
On Dec 18, 2009, at 11:53 AM, David Greene wrote:> On Friday 18 December 2009 12:28, Chris Lattner wrote: > >> This is looking a lot better, here are some more comments: >>> + /// current_pos - Return the current position within the >>> stream, >>> + /// not counting the bytes currently in the buffer. >>> + virtual uint64_t current_pos() { >> >> I didn't notice this earlier, but is there any reason for >> current_pos to be >> non-const in raw_ostream? > > Probably not. I'm going to let someone else fix that, though.Alright, I'll do it.>> How about just asserting that BufferSize is not zero? This stream >> won't >> work well and doesn't make sense with a buffer of zero. > > Sure it does. In fact it's crucial for making dbgs() work like > errs() in > the default case.Ah, I forgot the usage model.>> Is this to debug the implementation of the stream? Should this go >> into the >> tree? > > No, this is what gets dumped at program termination. Remember, this > stream > buffers everything you send it, effectively dropping everything > except the > last N characters, when N == BufferSize. Then at program > termination it > dumps the buffer.Aha, ok. So the usage model is that it normally is unbuffered and pass through like errs(), but you can pass a flag that causes all the - debug output to be circular buffered?> logs() might be a better name than dbgs() for this. What do you > think?If it writes out to stderr, I think dbgs() is a better name, logs() implies that it writes to a log file (at least to me). -Chris
On Friday 18 December 2009 19:24, Chris Lattner wrote:> > No, this is what gets dumped at program termination. Remember, this > > stream > > buffers everything you send it, effectively dropping everything > > except the > > last N characters, when N == BufferSize. Then at program > > termination it > > dumps the buffer. > > Aha, ok. So the usage model is that it normally is unbuffered and > pass through like errs(), but you can pass a flag that causes all the - > debug output to be circular buffered?Right!> > logs() might be a better name than dbgs() for this. What do you > > think? > > If it writes out to stderr, I think dbgs() is a better name, logs() > implies that it writes to a log file (at least to me).Fair enough. -Dave