On Wednesday 16 December 2009 13:35, David Greene wrote:> > Please make BufferSize an 'unsigned' and default it to 8192. Please use > > PRESERVE_STREAM instead of 'false'.Here's an updated version of the circular buffer. Ok to check in? -Dave Index: include/llvm/Support/circular_raw_ostream.h ==================================================================--- include/llvm/Support/circular_raw_ostream.h (revision 0) +++ include/llvm/Support/circular_raw_ostream.h (revision 0) @@ -0,0 +1,150 @@ +//===-- llvm/Support/circular_raw_ostream.h - Buffered streams --*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file contains raw_ostream implementations for streams to do circular +// buffering of their output. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_SUPPORT_CIRCULAR_RAW_OSTREAM_H +#define LLVM_SUPPORT_CIRCULAR_RAW_OSTREAM_H + +#include "llvm/Support/raw_ostream.h" + +namespace llvm +{ + /// circular_raw_ostream - A raw_ostream that saves its output in a + /// circular buffer. + /// + class circular_raw_ostream : public raw_ostream { + public: + /// 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. + /// + unsigned 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. 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. + /// + /// 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]; + 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,45 @@ +//===- 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" + +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) { + while (Size > 0 && Cur != BufferArray + BufferSize) { + --Size; + *Cur++ = *Ptr++; + } + 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 17, 2009, at 3:41 PM, David Greene wrote:> On Wednesday 16 December 2009 13:35, David Greene wrote: > >>> Please make BufferSize an 'unsigned' and default it to 8192. Please use >>> PRESERVE_STREAM instead of 'false'. > > Here's an updated version of the circular buffer. Ok to check in?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?> + /// 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.> + /// > + /// 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.> > + 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. It seems reasonable to say (and add to the doxygen comment for the default ctor) that the only operations valid on a default constructed stream are destruction and setStream.> + 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. 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? ..> +void circular_raw_ostream::write_impl(const char *Ptr, size_t Size) { > + if (BufferSize > 0) {This check can go away.> + // Write into the buffer, wrapping if necessary. > + while (Size > 0) { > + while (Size > 0 && Cur != BufferArray + BufferSize) { > + --Size; > + *Cur++ = *Ptr++; > + }This should use memcpy.> +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? -Chris
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