RFR: 7164841: Improvements to the GC log file rotation
Thomas Schatzl
thomas.schatzl at oracle.com
Mon Sep 16 17:34:17 UTC 2013
Hi Yumin,
sorry for the delays...
On Thu, 2013-09-12 at 15:57 -0700, Yumin Qi wrote:
> Thomas,
>
> This is the new version:
> http://cr.openjdk.java.net/~minqi/7164841/webrev05
>
> The change is adding internal tests after make_log_name_internal
> added to implement the original make_log_name function.
>
> Please let me know if it is OK. JPRT task is under building/testing,
> so far it is good.
Looks good.
Some minor things:
- can you add a test for a string not containing any of the markers?
(E.g. I used
// %.log
jio_snprintf(i_result, sizeof(char)*FILENAMEBUFLEN, "%%.log", pid);
o_result = make_log_name_internal("%.log", NULL, pid, tms);
assert(strcmp(i_result, o_result)==0, "failed on testing make_log_name(\"%.log\", NULL)");
FREE_C_HEAP_ARRAY(char, o_result, mtInternal);
// empty string
jio_snprintf(i_result, sizeof(char)*FILENAMEBUFLEN, "", pid);
o_result = make_log_name_internal("", NULL, pid, tms);
assert(strcmp(i_result, o_result)==0, "failed on testing make_log_name(\"\", NULL)");
FREE_C_HEAP_ARRAY(char, o_result, mtInternal);
// %%p.log
jio_snprintf(i_result, sizeof(char)*FILENAMEBUFLEN, "%%pid%u.log", pid);
o_result = make_log_name_internal("%%p.log", NULL, pid, tms);
assert(strcmp(i_result, o_result)==0, "failed on testing make_log_name(\"%%p.log\", NULL)");
FREE_C_HEAP_ARRAY(char, o_result, mtInternal);
which pass btw)
- in jni.cpp, the new include of ostream.hpp should probably be added to
the ones private to the testing code (e.g. jni.cpp line ~5040).
- in the string constant beginning in line 2886 I would prefer having if
no line started with a space - but that's really picky.
- in the tests the comparison of the result of strcmp to equal 0 should
probably have space before and after the "=="
Thanks,
Thomas
More information about the hotspot-gc-dev
mailing list