stringStream in UL and nested ResourceMarks
marcus.larsson at oracle.com
Tue Jun 13 08:32:43 UTC 2017
On 2017-06-12 17:47, Thomas Stüfe wrote:
> Okay, I put a bit of work into it (stack-only LogStream) and
> encountered nothing difficult, just a lot of boilerplate work.
> This is how I am changing the callsites:
> Please tell me if you had something different in mind; I want to make
> sure we have the same idea before changing such a lot of code.
Nice work. This is what I expected to see.
Just a small suggestion I thought of when looking at it:
769 Log(itables) logi;
770 LogStream lsi(logi.trace());
you could do
769 LogStream lsi(Log(itables)::trace());
to save a line.
Of course this only works if the log instance isn't used for anything
else than initializing the LogStream.
Touching on this is a suggestion that was brought up here when
discussing this locally, which is to add is_enabled() functions to the
By doing that we wouldn't need the log instance at all in places where
only the stream API is used, which is good.
If we're going for that, it would be great to do it before this change
so we could fix up the call sites to use that as well.
Let me know what you think.
> - I only remove ResourceMark objects where I can be sure that they are
> intended to scope the LogStream itself. If I cannot be sure, I leave
> them in for now.
> - using LogStream as a stack object means we need to include
> logStream.hpp (or decide to move LogStream to log.hpp).
I don't think it's a big deal to include logStream.hpp as well, but we
can consider moving it into log.hpp if people prefer that. I don't
really have a strong opinion on this.
> - If there is only one debug level involved (true in a lot of places),
> I prefer LogTarget to Log, because it minimizes error possibilities
> - In some places the coding gets more complicated, usually if the code
> wants to decide the log level dynamically, but the log level is a
> template parameter to LogTarget. See e.g. linkresolver.cpp.
> - I see the macro "log_develop_is_enabled" and wonder why it exists
> and why the code is not just surrounded by #ifndef PRODUCT?
That's only for convenience. It could as well have been ifndef:ed.
More information about the hotspot-dev