[PATCH] Disable buffering for raw_null_ostream()

[Mehdi Amini]

> 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?

> `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?

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

— 
Mehdi



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