<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi JC,<div class=""><br class=""></div><div class="">thanks for reviewing it!<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Oct 9, 2018, at 1:47 PM, JC Beyler <<a href="mailto:jcbeyler@google.com" class="">jcbeyler@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class="">Hi Igor,<div class=""><br class=""></div><div class="">I was looking at this and it looks good to me (not a reviewer of course) but I noticed a few nits:</div><div class=""><br class=""></div><div class="">- <a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/src/hotspot/share/gc/shared/gcTimer.cpp.udiff.html" class="">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/src/hotspot/share/gc/shared/gcTimer.cpp.udiff.html</a></div><div class=""> - Seems like you could remove a few more lines there because now you have empty lines at the end of the file</div></div></div></div></div></div></div></blockquote>fixed<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">- <a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/test/hotspot/gtest/gc/shared/test_gcTimer.cpp.html" class="">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/test/hotspot/gtest/gc/shared/test_gcTimer.cpp.html</a></div><div class=""> - Should the copyright year 2001 be here?</div></div></div></div></div></div></div></blockquote>yes, as this code was originally introduced in 2001 as part of gcTimer.cpp <br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class=""> - Indentation is a bit off:</div><div class=""> - <span style="" class="">GCTimerTest has 2 spaces for public and 4 for methods</span></div><div class=""><span style="" class=""> - </span><span style="" class="">TimePartitionPhasesIteratorTest has 1 and 2</span></div><div class=""><span style="" class=""> - Tests afterwards have 2</span></div></div></div></div></div></div></div></blockquote>dough, that's what happens when you copy-paste someone's else code... fixed. now GCTimerTest and TimePartitionPhasesIteratorTest have 1 for public and 2 for methods, all tests have 2. </div><div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class=""><span style="" class=""><br class=""></span></div><div class=""><span style="" class=""> - The code could be simplified if we removed the </span><font class="">TimePartitionPhasesIteratorTest entirely and just put it in an anonymous namespace</font></div></div></div></div></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class=""><font class=""> - hotspot/gtest/utilities/test_bitMap_search.cpp uses anonymous namespaces so gtest does support it</font><br class=""></div><div class=""><font class=""> Then lines such as:</font></div><div class=""><pre style="" class=""> 219 EXPECT_NO_FATAL_FAILURE(TimePartitionPhasesIteratorTest::validate_gc_phase(iter.next(), 1, "SubPhase3", 15, 16));</pre></div><div class=""><font class="">become:</font></div><div class=""><pre style="" class=""> 219 EXPECT_NO_FATAL_FAILURE(validate_gc_phase(iter.next(), 1, "SubPhase3", 15, 16));</pre></div></div></div></div></div></div></div></blockquote>unfortunately we can't do it, TimePartitionPhasesIteratorTest (as well as GCTimerTest) is needed b/c it's a friend of TimeInstant (Ticks), so it can use private c-tor TimeInstant(jlong).</div><div><br class=""></div><div>-- Igor<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div class=""><pre style="" class="">Apart from that, it looks good to me :)<br class=""></pre><pre style="" class="">Jc</pre><pre style="" class=""><br class=""></pre></div></div></div></div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Tue, Oct 9, 2018 at 10:24 AM Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com" class="">igor.ignatyev@oracle.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html" rel="noreferrer" target="_blank" class="">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html</a><br class="">
> 450 lines changed: 238 ins; 211 del; 1 mod;<br class="">
<br class="">
Hi all,<br class="">
<br class="">
could you please review this small (and hopefully trivial) patch which converts internal GCTimer_test to gtest?<br class="">
<br class="">
webrev: <a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html" rel="noreferrer" target="_blank" class="">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html</a><br class="">
JBS: <a href="https://bugs.openjdk.java.net/browse/JDK-8157728" rel="noreferrer" target="_blank" class="">https://bugs.openjdk.java.net/browse/JDK-8157728</a><br class="">
testing:<br class="">
- converted tests on linux-x64, windows-x64, macosx-x64, solaris-sparcv9 in product and fastdebug variants <br class="">
- build w/ precompiled-headers enabled and disabled<br class="">
<br class="">
PS the patch has been originally created by Kirill Zh, but hasn't been sent out for official review<br class="">
<br class="">
Thanks,<br class="">
-- Igor <br class="">
</blockquote></div><br clear="all" class=""><div class=""><br class=""></div>-- <br class=""><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr" class=""><div class=""><br class=""></div>Thanks,<div class="">Jc</div></div></div>
</div></blockquote></div><br class=""></div></body></html>