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

[Thomas Stüfe]

On Tue 21. May 2019 at 21:29, Kim Barrett <kim.barrett at oracle.com> 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...


That is what I meant too. I try to limit “knowledge” about things where it
is not needed to keep the code flexible.


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

Great thanks!

..thomas

>
>