[LLVMdev] [PATCH] Circular Buffered Debug Stream

[Chris Lattner]

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