<div dir="ltr">Good point on the friend class, looks good to me then :)<br><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 2:03 PM Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com">igor.ignatyev@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space">Hi JC,<div><br></div><div>thanks for reviewing it!<br><div><br><blockquote type="cite"><div>On Oct 9, 2018, at 1:47 PM, JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>> wrote:</div><br class="m_5955712651882605032Apple-interchange-newline"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Igor,<div><br></div><div>I was looking at this and it looks good to me (not a reviewer of course) but I noticed a few nits:</div><div><br></div><div>- <a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/src/hotspot/share/gc/shared/gcTimer.cpp.udiff.html" target="_blank">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/src/hotspot/share/gc/shared/gcTimer.cpp.udiff.html</a></div><div> - 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><blockquote type="cite"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div>- <a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/test/hotspot/gtest/gc/shared/test_gcTimer.cpp.html" target="_blank">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/test/hotspot/gtest/gc/shared/test_gcTimer.cpp.html</a></div><div> - 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><blockquote type="cite"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div> - Indentation is a bit off:</div><div> - <span>GCTimerTest has 2 spaces for public and 4 for methods</span></div><div><span> - </span><span>TimePartitionPhasesIteratorTest has 1 and 2</span></div><div><span> - 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"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><span><br></span></div><div><span> - The code could be simplified if we removed the </span><font>TimePartitionPhasesIteratorTest entirely and just put it in an anonymous namespace</font></div></div></div></div></div></div></div></blockquote><blockquote type="cite"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><font> - hotspot/gtest/utilities/test_bitMap_search.cpp uses anonymous namespaces so gtest does support it</font><br></div><div><font> Then lines such as:</font></div><div><pre> 219 EXPECT_NO_FATAL_FAILURE(TimePartitionPhasesIteratorTest::validate_gc_phase(iter.next(), 1, "SubPhase3", 15, 16));</pre></div><div><font>become:</font></div><div><pre> 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></div><div>-- Igor<br><blockquote type="cite"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><pre>Apart from that, it looks good to me :)<br></pre><pre>Jc</pre><pre><br></pre></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 9, 2018 at 10:24 AM Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com" target="_blank">igor.ignatyev@oracle.com</a>> wrote:<br></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">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html</a><br>
> 450 lines changed: 238 ins; 211 del; 1 mod;<br>
<br>
Hi all,<br>
<br>
could you please review this small (and hopefully trivial) patch which converts internal GCTimer_test to gtest?<br>
<br>
webrev: <a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html</a><br>
JBS: <a href="https://bugs.openjdk.java.net/browse/JDK-8157728" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8157728</a><br>
testing:<br>
- converted tests on linux-x64, windows-x64, macosx-x64, solaris-sparcv9 in product and fastdebug variants <br>
- build w/ precompiled-headers enabled and disabled<br>
<br>
PS the patch has been originally created by Kirill Zh, but hasn't been sent out for official review<br>
<br>
Thanks,<br>
-- Igor <br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="m_5955712651882605032gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>
</div></blockquote></div><br></div></div></blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>