<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">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><br></div><div>- <a href="http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/test/hotspot/gtest/gc/shared/test_gcTimer.cpp.html">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><br></div><div> - Indentation is a bit off:</div><div> - <span style="color:rgb(0,0,0)">GCTimerTest has 2 spaces for public and 4 for methods</span></div><div><span style="color:rgb(0,0,0)"> - </span><span style="color:rgb(0,0,0)">TimePartitionPhasesIteratorTest has 1 and 2</span></div><div><span style="color:rgb(0,0,0)"> - Tests afterwards have 2</span></div><div><span style="color:rgb(0,0,0)"><br></span></div><div><span style="color:rgb(0,0,0)"> - The code could be simplified if we removed the </span><font color="#000000">TimePartitionPhasesIteratorTest entirely and just put it in an anonymous namespace</font></div><div><font color="#000000"> - hotspot/gtest/utilities/test_bitMap_search.cpp uses anonymous namespaces so gtest does support it</font><br></div><div><font color="#000000"> Then lines such as:</font></div><div><pre style="color:rgb(0,0,0)"> 219 EXPECT_NO_FATAL_FAILURE(TimePartitionPhasesIteratorTest::validate_gc_phase(iter.next(), 1, "SubPhase3", 15, 16));</pre></div><div><font color="#000000">become:</font></div><div><pre style="color:rgb(0,0,0)"> 219 EXPECT_NO_FATAL_FAILURE(validate_gc_phase(iter.next(), 1, "SubPhase3", 15, 16));</pre><pre style="color:rgb(0,0,0)"><br></pre><pre style="color:rgb(0,0,0)">Apart from that, it looks good to me :)<br></pre><pre style="color:rgb(0,0,0)">Jc</pre><pre style="color:rgb(0,0,0)"><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">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="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>