RFR(S) : 8157728 : Covert GCTimer_test to GTest
igor.ignatyev at oracle.com
Tue Oct 9 21:03:46 UTC 2018
thanks for reviewing it!
> On Oct 9, 2018, at 1:47 PM, JC Beyler <jcbeyler at google.com> wrote:
> Hi Igor,
> I was looking at this and it looks good to me (not a reviewer of course) but I noticed a few nits:
> - 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>
> - Seems like you could remove a few more lines there because now you have empty lines at the end of the file
> - 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>
> - Should the copyright year 2001 be here?
yes, as this code was originally introduced in 2001 as part of gcTimer.cpp
> - Indentation is a bit off:
> - GCTimerTest has 2 spaces for public and 4 for methods
> - TimePartitionPhasesIteratorTest has 1 and 2
> - Tests afterwards have 2
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.
> - The code could be simplified if we removed the TimePartitionPhasesIteratorTest entirely and just put it in an anonymous namespace
> - hotspot/gtest/utilities/test_bitMap_search.cpp uses anonymous namespaces so gtest does support it
> Then lines such as:
> 219 EXPECT_NO_FATAL_FAILURE(TimePartitionPhasesIteratorTest::validate_gc_phase(iter.next(), 1, "SubPhase3", 15, 16));
> 219 EXPECT_NO_FATAL_FAILURE(validate_gc_phase(iter.next(), 1, "SubPhase3", 15, 16));
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).
> Apart from that, it looks good to me :)
> On Tue, Oct 9, 2018 at 10:24 AM Igor Ignatyev <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
> http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html>
> > 450 lines changed: 238 ins; 211 del; 1 mod;
> Hi all,
> could you please review this small (and hopefully trivial) patch which converts internal GCTimer_test to gtest?
> webrev: http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html <http://cr.openjdk.java.net/~iignatyev//8157728/webrev.00/index.html>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8157728 <https://bugs.openjdk.java.net/browse/JDK-8157728>
> - converted tests on linux-x64, windows-x64, macosx-x64, solaris-sparcv9 in product and fastdebug variants
> - build w/ precompiled-headers enabled and disabled
> PS the patch has been originally created by Kirill Zh, but hasn't been sent out for official review
> -- Igor
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev