[PATCH] Disable buffering for raw_null_ostream()

[Mehdi Amini]

> On Jul 1, 2015, at 2:30 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2015-Jul-01, at 13:35, Mehdi Amini <mehdi.amini at apple.com> wrote:
>> 
>>> 
>>> On Jul 1, 2015, at 12:25 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>>> 
>>>> On 2015-Jul-01, at 12:12, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>> 
>>>>> 
>>>>> On Jul 1, 2015, at 12:05 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>> 
>>>>> 
>>>>>> On 2015-Jun-30, at 22:40, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>>>>>> 
>>>>>> Hi rafael,
>>>>>> 
>>>>>> There is no need to buffer the nulls() output.
>>>>>> Moreover it kept a shared buffer, and made using nulls() not possible
>>>>>> in a multi-threaded environment.
>>>>>> 
>>>>>> http://reviews.llvm.org/D10861
>>>>>> 
>>>>>> Files:
>>>>>> include/llvm/Support/raw_ostream.h
>>>>>> 
>>>>>> Index: include/llvm/Support/raw_ostream.h
>>>>>> ===================================================================
>>>>>> --- include/llvm/Support/raw_ostream.h
>>>>>> +++ include/llvm/Support/raw_ostream.h
>>>>>> @@ -530,7 +530,7 @@
>>>>>> uint64_t current_pos() const override;
>>>>>> 
>>>>>> public:
>>>>>> -  explicit raw_null_ostream() {}
>>>>>> +  explicit raw_null_ostream() : raw_pwrite_stream(true) {}
>>>>> 
>>>>> Can you document the `true` here?
>>>>> 
>>>>> : raw_pwrite_stream(/* Unbuffered */ true) {}
>>>> 
>>>> What about changing the bool to an enum class instead?
>>>> I don’t really like boolean argument for this reason.
>>> 
>>> Might be overkill; we use `bool`s as args all over.  
>> 
>> Does not mean it is a good thing :)
>> Any down side to use enum instead?
> 
> There's no good way to add an implicit conversion to an enum class,
> and it requires more boiler-plate.  Sometimes enum classes are worth
> the trouble, sometimes only enums are, and sometimes really you just
> want a `bool`.
> 
> Given that the only reasonable options for these functions are
> `InternalBuffer` and `Unbuffered`, a `bool` is arguably more clear
> in this case.
> 
>> 
>>> `bool`s are
>>> awkward as arguments, but IMO documenting them is usually sufficient.
>> 
>> I don’t like potential for confusion, for example when reading:
>> 
>> raw_pwrite_stream(/* Unbuffered */ false) 
>> 
>> Does it mean that “unbuffered” is false and then the stream is buffered?
>> Or does the false mean that buffered is disabled and the “unbuffered” comment makes explicit that the programmer except the mode to be set to unbuffer?
> 
> Common practice in LLVM is to exactly duplicate the parameter name,
> as if it's a named paremeter (read `Unbuffered: false`).  If you see
> anywhere that does it differently, please fix it.
> 
> Of course, the `Unbuffered` parameter is spelled wrong -- our coding
> standards would recommend `IsUnbuffered` -- and if it were named
> correctly I suspect you'd find it a little more obvious.  Or just
> say `/* Unbuffered = */ false`, that's common too.

Still error prone and not tool friendly.
`StreamMode::Unbuffered ` is at least as much readable as the inline comment, with the advantage that clients can't omit the comment (or more wicked: reverse the comment), and it is more tool-friendly (autocompletion for instance).


> 
>> 
>> Admittedly the confusion is emphasized by the fact that the boolean is associated to a “negated” argument, if you see the stream has “having or not a buffer” there is a double negation to enable the buffer.
>> 
>> (Revision updated BTW).
> 
> Why isn't it attached to the same email?  Anyway, I found it.
> 
>>> 
>>> As long as it doesn't conflict strangely with `BufferKind` I don't
>>> see a problem with it though (as long as it's a separate commit from
>>> the functional change).
> 
> It looks like you've done both in the same patch.  Please separate into
> the two patches -- one to change the API, and another to change
> semantics.

I have two local commits, but since the raw_null_ostream is a “one-liner” I thought it was not worth opening another revision on Phabricator.
(Phabricator shows that there are two local separate commits but unfortunately does not allow to split the diff for only one of them)

I created a new revision for the refactoring commit: http://reviews.llvm.org/D10885 ; it should address your comment below.

— 
Mehdi



> 
> Regarding the API change part of the patch:
> 
>> Index: include/llvm/Support/raw_ostream.h
>> ===================================================================
>> --- include/llvm/Support/raw_ostream.h
>> +++ include/llvm/Support/raw_ostream.h
>> @@ -60,11 +60,12 @@
>>   /// this buffer.
>>   char *OutBufStart, *OutBufEnd, *OutBufCur;
>> 
>> -  enum BufferKind {
>> -    Unbuffered = 0,
>> -    InternalBuffer,
>> -    ExternalBuffer
>> -  } BufferMode;
>> +public:
>> +  /// Specify the current state of the stream.
>> +  enum class BufferKind { Unbuffered, InternalBuffer, ExternalBuffer };
> 
> Should this be `protected`?
> 
> Is there any benefit to the `class` here?  The enum is already in the
> `raw_ostream` namespace, and it looks like this causes a lot of churn.
> 
>> +
>> +private:
>> +  BufferKind BufferMode;
>> 
>> public:
>>   // color order matches ANSI escape sequence, don't change
>> @@ -102,13 +103,13 @@
>>   /// Set the stream to be buffered, using the specified buffer size.
>>   void SetBufferSize(size_t Size) {
>>     flush();
>> -    SetBufferAndMode(new char[Size], Size, InternalBuffer);
>> +    SetBufferAndMode(new char[Size], Size, BufferKind::InternalBuffer);
> 
> ... like here.
> 
>>   }
>> 
>>   size_t GetBufferSize() const {
>>     // If we're supposed to be buffered but haven't actually gotten around
>>     // to allocating the buffer yet, return the value that would be used.
>> -    if (BufferMode != Unbuffered && OutBufStart == nullptr)
>> +    if (BufferMode != BufferKind::Unbuffered && OutBufStart == nullptr)
> 
> (etc.)
> 
>>       return preferred_buffer_size();
>> 
>>     // Otherwise just return the size of the allocated buffer.
>> @@ -320,7 +321,7 @@
>>   virtual void pwrite_impl(const char *Ptr, size_t Size, uint64_t Offset) = 0;
>> 
>> public:
>> -  explicit raw_pwrite_stream(bool Unbuffered = false)
>> +  explicit raw_pwrite_stream(BufferKind Unbuffered = BufferKind::InternalBuffer)
>>       : raw_ostream(Unbuffered) {}
> 
> `Unbuffered` looks like the wrong variable name.
> 
>>   void pwrite(const char *Ptr, size_t Size, uint64_t Offset) {
>> #ifndef NDBEBUG
>> @@ -387,7 +388,8 @@
>> 
>>   /// FD is the file descriptor that this writes to.  If ShouldClose is true,
>>   /// this closes the file when the stream is destroyed.
>> -  raw_fd_ostream(int fd, bool shouldClose, bool unbuffered=false);
>> +  raw_fd_ostream(int fd, bool shouldClose,
>> +                 BufferKind Unbuffered = BufferKind::InternalBuffer);
> 
> Also a bad name here.
> 
>> 
>>   ~raw_fd_ostream() override;
>> 
>> Index: lib/Bitcode/Writer/BitWriter.cpp
>> ===================================================================
>> --- lib/Bitcode/Writer/BitWriter.cpp
>> +++ lib/Bitcode/Writer/BitWriter.cpp
>> @@ -30,7 +30,10 @@
>> 
>> int LLVMWriteBitcodeToFD(LLVMModuleRef M, int FD, int ShouldClose,
>>                          int Unbuffered) {
>> -  raw_fd_ostream OS(FD, ShouldClose, Unbuffered);
>> +  raw_ostream::BufferKind Mode = Unbuffered
> 
> Ah, probably it can't be `protected`.
> 
>> +                                     ? raw_ostream::BufferKind::Unbuffered
> 
> The extra namespace seems pretty awkward here.
> 
>> +                                     : raw_ostream::BufferKind::InternalBuffer;
> 
> Should the caller really be exposed to whether this is an "internal" or
> "external" buffer?  There seems to be new potential for misuse here...
> maybe the `bool` was actually safer.
> 
>> +  raw_fd_ostream OS(FD, ShouldClose, Mode);
>> 
>>   WriteBitcodeToFile(unwrap(M), OS);
>>   return 0;