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

[Thomas Stüfe]

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.


> > src/hotspot/share/utilities/events.hpp
> > Removed from class FormatStringLogMessage
> >  140  public:
> >  141   // Wrap this buffer in a stringStream.
> >  142   stringStream stream() {
> >  143     return stringStream(this->_buf, this->size());
> >  144   }
> >
> > FormatStringLogMessage seems kind of pointless now.  Maybe it should
> > be eliminated?  That could be a separate cleanup.
> >
> >
> > Yes. As I wrote, I have a larger patch under review which changes all
> that code anyway.
> >
> > If I just remove FormatStringMessage I must publish the ctor of the
> parent class, FormatMessage, and that was just that one step too far for
> this patch.
>
> I suspected as much; thanks for confirming.
>
>