RFR(xs): 8224487: outputStream should not be copyable

[coleen.phillimore at oracle.com]

Thomas, This change looks good to me.
thanks,
Coleen

On 5/21/19 3:29 PM, Kim Barrett wrote:
>> On May 21, 2019, at 2:28 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>
>>
>>
>> On Tue, May 21, 2019 at 8:23 PM Kim Barrett <kim.barrett at oracle.com> wrote:
>>> On May 21, 2019, at 12:20 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>> src/hotspot/share/utilities/exceptions.hpp
>>>   189   static void log_exception(Handle exception, stringStream tempst);
>>> =>
>>>   189   static void log_exception(Handle exception, const char* message);
>>>
>>> Why not change the stringStream parameter to be passed by const-ref
>>> rather than by value?  That would avoid changing all the callers.
>>>
>>>
>>> Because handing the whole stream object down to log_exception was overkill anyway. All it wants is to print a text message, so I just give it that and live with a slightly larger patch diff.
>>>
>>> Also, I am not a big fan of references but that is just me. I usually prefer to see at the call site if I hand in value or reference without looking up the prototype.
>> [Oops, I missed the bug introduced into log_exception by webrev.00;  I
>> was focused on the question of the parameter change.]
>>
>>
>> Yes me too :( Shouldn't have renamed that thing in the first place. Thanks to jdk-submit tests I still got that.
>>   
>> I disagree, for a number of reasons, but mostly because passing by
>> const-ref is completely normal for C++.  One doesn't need to look at
>> the prototype to determine value vs const-ref passing, because from
>> the caller's POV it doesn't matter.
>>
>>
>> Okay, I see your point.
>>
>> Are you still okay with this version? My original reasoning still holds - a const char* is sufficient at this place, log_exception does not need to take the whole stringStream.
>>   
> After thinking about it some more, yes, though not *precisely* for that reason.
> Rather, it seems a bit odd that there should be any knowledge of stringStream
> in this API.  So...
>
>> delta: http://cr.openjdk.java.net/~stuefe/webrevs/8224487-make-streams-not-copyable/webrev_delta.01/webrev/
>> full: http://cr.openjdk.java.net/~stuefe/webrevs/8224487-make-streams-not-copyable/webrev.01/webrev/
>>
> Looks good.
>