[PATCH] Disable buffering for raw_null_ostream()

[Duncan P. N. Exon Smith]

> On 2015-Jul-01, at 15:32, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
>> 
>> 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)

If Phabricator makes it difficult to send a review, then I'd recommend
not using Phabricator for that review...

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

Sure, I'll look for that thread.