[llvm-commits] [llvm] r75883 - /llvm/trunk/include/llvm/Support/FormattedStream.h

[Daniel Dunbar]

Minor comments you happened to get stuck with:

On Wed, Jul 15, 2009 at 6:32 PM, Dan Gohman<gohman at apple.com> wrote:
> Author: djg
> Date: Wed Jul 15 20:32:46 2009
> New Revision: 75883
>
> URL: http://llvm.org/viewvc/llvm-project?rev=75883&view=rev
> Log:
> formatted_raw_ostream both is-a raw_ostream and has-a raw_ostream. This
> means that two separate raw_ostreams are doing buffering before data is
> sent to the underlying stream. Besides the inefficiency of redundant
> buffering, the second level of buffering doesn't recieve flush()
> requests.
>
> Fix this by having formatted_raw_ostream set the underlying raw_ostream
> to be unbuffered. This eliminates inefficiency due to redundant buffering,
> and it makes the flush() disconnect harmless.
>
> This fixes PR4559.
>
> Modified:
>    llvm/trunk/include/llvm/Support/FormattedStream.h
>
> Modified: llvm/trunk/include/llvm/Support/FormattedStream.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FormattedStream.h?rev=75883&r1=75882&r2=75883&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/FormattedStream.h (original)
> +++ llvm/trunk/include/llvm/Support/FormattedStream.h Wed Jul 15 20:32:46 2009
> @@ -33,7 +33,8 @@
>     const static bool PRESERVE_STREAM = false;
>
>   private:
> -    /// TheStream - The real stream we output to.
> +    /// 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
> @@ -70,13 +71,27 @@
>     /// 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 formatted_raw_ostream does its own buffering,
> +    /// so it doesn't want another layer of buffering to be happening
> +    /// underneath it.
> +    ///
>     /// \param Filename - The file to open. If this is "-" then the
>     /// stream will use stdout instead.

There is no filename argument.

>     /// \param Binary - The file should be opened in binary mode on
>     /// platforms that support this distinction.
>     formatted_raw_ostream(raw_ostream &Stream, bool Delete = false)
> -        : raw_ostream(), TheStream(&Stream), DeleteStream(Delete), Column(0) {}
> -    explicit formatted_raw_ostream()
> +      : raw_ostream(), TheStream(&Stream), DeleteStream(Delete), Column(0) {
> +      // This formatted_raw_ostream inherits from raw_ostream, so it'll do its
> +      // own buffering, and it doesn't need or want TheStream to do another
> +      // layer of buffering underneath. Resize the buffer to what TheStream
> +      // had been using, and tell TheStream not to do its own buffering.
> +      TheStream->flush();
> +      if (size_t BufferSize = TheStream->GetNumBytesInBuffer())
> +        SetBufferSize(BufferSize);
> +      TheStream->SetUnbuffered();

Can't this just call setStream?

> +    }
> +    explicit formatted_raw_ostream()
>       : raw_ostream(), TheStream(0), DeleteStream(false), Column(0) {}
>
>     ~formatted_raw_ostream() {
> @@ -87,6 +102,12 @@
>     void setStream(raw_ostream &Stream, bool Delete = false) {
>       TheStream = &Stream;
>       DeleteStream = Delete;
> +
> +      // Avoid double-buffering, as above.
> +      TheStream->flush();
> +      if (size_t BufferSize = TheStream->GetNumBytesInBuffer())
> +        SetBufferSize(BufferSize);
> +      TheStream->SetUnbuffered();

This routine should probably honor the existing Delete value, if it is
called multiple times?

 - Daniel