RFR(S) : 8157728 : Covert GCTimer_test to GTest

[Igor Ignatyev]

Hi JC,

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
fixed
> 
> - 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));
> become:
>  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).

-- Igor
> Apart from that, it looks good to me :)
> Jc
> 
> 
> 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>
> testing:
>  - 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
> 
> Thanks,
> -- Igor  
>  
> 
> 
> -- 
> 
> Thanks,
> Jc

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181009/c4fec2ff/attachment.htm>