Hi Kim,
On 03/22/2016 04:32 AM, Kim Barrett wrote:
On Mar 21, 2016, at 12:34 PM, Robbin Ehn <robbin....@oracle.com> wrote:
Hi all, please review this a somewhat bigger change-set.
Updated with the feedback.
New webrev: http://cr.openjdk.java.net/~rehn/8151993/v2/webrev/
Tested with jprt hotspot and I added 2 internal vm tests.
(also 2 bugs fixed, missing va_end and a potential race when calling prefix
function twice (very unlikely))
I prefer this new approach to what was in the original RFR. A few
comments:
------------------------------------------------------------------------------
src/share/vm/logging/log.hpp
91 class LogWrite : AllStatic {
I think the name LogWrite is too generic and potentially useful in the
future for this facility. I'd prefer something longer and more
descriptive, e.g. something like LogWriteLarge{Impl,Helper}.
Fixed
------------------------------------------------------------------------------
src/share/vm/logging/log.hpp
89 // Non-template helper class for implementing write-slowpath in cpp
90 template <LogTagType T0, LogTagType T1, LogTagType T2, LogTagType T3,
LogTagType T4, LogTagType GuardTag> class Log;
91 class LogWrite : AllStatic {
92 private:
93 template <LogTagType T0, LogTagType T1, LogTagType T2, LogTagType T3,
LogTagType T4, LogTagType GuardTag> friend class Log;
The really long lines and lack of whitespace here makes this hard to
read and confusing. It took several tries for me to spot the "class
Log" on line 90 and on line 93. Only knowing that the LogWrite class
and the write_large function couldn't possibly have template specs
eventually helped me find them.
So I think it would help readability to break up the declarations into
shorter lines with the "[friend] class Log" parts on their own line,
with some whitespace around them. And attach the "Non-template helper
class" comment to the LogWrite class rather than the forward
declaration.
Fixed
Aside: I keep wishing there were some macros for the various template
parameter sequences that are really just workarounds for the lack of
variadic templates. But that's a different problem.
------------------------------------------------------------------------------
src/share/vm/logging/log.hpp
95 static void write_large(LogTagSet& lts,
Passing in the tagset is much better than my suggestion of some
function pointers. Good.
------------------------------------------------------------------------------
src/share/vm/logging/log.hpp
156 va_end(saved_args);
Good catch.
------------------------------------------------------------------------------
src/share/vm/logging/log.cpp
Thanks for new tests. However, I'm not sure it's safe to assume we can
create and remove files from the current directory when running these
tests. Maybe mkstemp with a file in os::get_temp_directory()? Or maybe
there's something like this already packaged up in hotspot? tmpfile
would be ideal if there were a way to configure logging to use a file
descriptor, but I don't see anything like that.
I used "os::get_temp_directory()" but generated 'naive' filename using
pid and test name.
Using mkstemp and from fd lookup filename we need os dependent code, so
I skipped this.
(we could use tempnam in this case, but it creates a warning since it's
deprecated)
------------------------------------------------------------------------------
src/share/vm/logging/log.hpp
148 int msg_len = os::log_vsnprintf(buf + prefix_len, sizeof(buf) -
prefix_len, fmt, args);
149 assert(msg_len >= 0, "Log message buffer issue");
I think I would prefer the pre-change
int res = ...;
assert(res >= 0, ...);
followed by
size_t msg_len = res;
and eliminate the casts of msg_len. But I dislike casts...
Fixed
------------------------------------------------------------------------------
Here is the webrev: http://cr.openjdk.java.net/~rehn/8151993/v3/webrev/
Btw what's the normal workflow creating incrementals using hg mq ?
Thanks !
/Robbin
