RFR(xs): 8151993: Remove inclusion of inline.hpp in log.hpp

[Robbin Ehn]

Hi Kim,

On 03/22/2016 04:32 AM, Kim Barrett wrote:
>> On Mar 21, 2016, at 12:34 PM, Robbin Ehn <robbin.ehn at 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