RFR(S) : 8157728 : Covert GCTimer_test to GTest

[JC Beyler]

Good point on the friend class, looks good to me then :)
Jc

On Tue, Oct 9, 2018 at 2:03 PM Igor Ignatyev <igor.ignatyev at oracle.com>
wrote:

> 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
>    - 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
>    - 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>
> wrote:
>
>> 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
>> JBS: 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
>
>
>

-- 

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