[LLVMdev] [PATCH] Circular Buffered Debug Stream

[Chris Lattner]

On Dec 18, 2009, at 3:46 PM, David Greene wrote:
>
> +  /// circular_raw_ostream - A raw_ostream that saves its output in a
> +  /// circular buffer.

A better description would be "which *can* save its data to a circular  
buffer, or can pass it through directly to an underlying stream if  
specified with a buffer of zero."

When it is buffering, what causes it to flush?  Your current model  
requires "printLog" which is not an obvious name for this, would it be  
better to do it on destruction?  How about using something like  
flush_to_underlying_stream() or something like that?

> +    /// 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;


I don't find these to be very clear names.  How about giving them a  
name involving 'ownership' like TAKES_OWNERSHIP?  "preservation" isn't  
really the same as "not deleting when the stream is destroyed" to me.


> +    /// TheStream - The real stream we output to. We set it to be
> +    /// unbuffered, since we're already doing our own buffering.
> +    ///
> +    raw_ostream *TheStream;

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.

>
> +    /// printLog - Dump the contents of the buffer to Stream.
> +    ///
> +    void printLog(void) {
> +      TheStream->write(BufferArray, BufferSize);
> +      Cur = BufferArray;
> +    }

If the buffer has circled around, won't this print it out-of-order?   
If the current pointer is 5 and the length is 10, it seems like this  
should do a write([5..9]) then write([0..4])?

> +    /// current_pos - Return the current position within the stream,
> +    /// not counting the bytes currently in the buffer.
> +    virtual uint64_t current_pos() {

This is now const on mainline.

> +  public:
> +    /// circular_raw_ostream - Open the specified file for
> +    /// writing.

This is not a file, please read the comments you're copying :)

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

Again, I don't think this should happen anymore.  You should add some  
comment about what Delete does, and change it to not use 'false' as I  
asked for before.

> +    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)

Please change all these predicates to BufferSize != 0 instead of using  
 >.

> +
> +#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) {

Please use:

if (BufferSize == 0) {
   .. write ..
   return;
}

to reduce nesting.

> +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();
> +  }
> +}

I'm still unclear about this and printLog.  The "Log Output" prefix is  
confusing to me, because this isn't a log file.  Maybe just remove the  
prefix?

-Chris