RFR(S): 8164230: Convert TestCodeCacheRemSet_test to GTest

[Mikael Gerdin]

Hi Kirill,

On 2016-08-29 19:28, Kirill Zhaldybin wrote:
> Mikael,
>
> Here are a new WebRev:
> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164230/webrev.02/
> Changes:
> 1. g1CodeRootSetTable.hpp - changed guard to
> SHARE_VM_GC_G1_G1CODEROOTSETTABLE_HPP
> 2. g1CodeRootSetTable.hpp - added forward declaration for class nmethod.
> 3. test_g1CodeCacheRemSet - changed
>
> 32 size_t threshold(G1CodeRootSet* root_set) {
> 33 return root_set->Threshold;
> 34 }
>
> to
> 32 size_t threshold() {
> 33 return G1CodeRootSet::Threshold;
> 34 }
> and changed corresponding code where threshold is called.
>
>
> Could you please let me know your opinion?

Looks good to me now, thanks!
/Mikael

>
> Thank you.
>
> Regards, Kirill
>
>
> On 26.08.2016 11:11, Mikael Gerdin wrote:
>> Hi Kirill,
>>
>> On 2016-08-22 20:40, Kirill Zhaldybin wrote:
>>> Mikael,
>>>
>>> Thank you for reviewing the fix!
>>>
>>> Here are a new WebRev:
>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164230/webrev.01/
>>
>> A few nits in g1CodeRootSetTable:
>> * the include guard still says _INTERNAL_HPP
>> * even though it sucessfully compiles right now I'd prefer if the
>> header forward declares or includes its obvious dependencies, in this
>> case those would just be a forward-declaration of "class nmethod"
>>
>> In test_g1CodeCacheRemSet
>>   size_t threshold(G1CodeRootSet* root_set) {
>>     return root_set->Threshold;
>>   }
>>
>> even though this compiles I would prefer if you referred to this
>> static member through the class name:
>> G1CodeRootSet::Threshold
>> That way you can avoid passing the instance as a parameter as well.
>>
>> Otherwise I think this change looks good.
>>
>> /Mikael
>>
>>
>>>
>>> Changes:
>>> 1. g1CodeCacheRemSet.internal.hpp renamed to g1CodeRootSetTable.hpp and
>>> the class was renamed to G1CodeRootSetTable, #include  added and I was
>>> able to build images and gtests with --disable-precompiled-headers.
>>>
>>> 2. Moved G1CodeRootSet to the test method
>>> 3. Renamed getters in G1CodeRootSetTest to snake_case.
>>>
>>> Could you please let me know your opinion?
>>>
>>> Thank you.
>>>
>>> Regards, Kirill
>>>
>>> On 22.08.2016 16:43, Mikael Gerdin wrote:
>>>> Hi Kirill,
>>>>
>>>> On 2016-08-22 14:57, Kirill Zhaldybin wrote:
>>>>> Dear all,
>>>>>
>>>>> Could you please review this fix for 8164230?
>>>>>
>>>>> Since we need CodeRootSetTable class to be declared in the test it was
>>>>> moved to new file src/share/vm/gc/g1/g1CodeCacheRemSet.internal.hpp.
>>>>> This file is included by both test_g1CodeCacheRemSet.cpp and
>>>>> g1CodeCacheRemSet.cpp.
>>>>
>>>> Please name the new file g1CodeRootSetTable.hpp and rename the class
>>>> G1CodeRootSetTable instead of inventing a new ".internal.hpp" format.
>>>> Also, the new hpp file does not properly #include its dependencies and
>>>> therefore the build fails if precompiled headers are disabled, please
>>>> add the appropriate includes and forward declarations in order to make
>>>> it complete.
>>>>
>>>>
>>>> The class G1CodeRootSetTest has a bunch of getters and a G1CodeRootSet
>>>> member, why are they like that instead of allocated on the stack like
>>>> the original version of the test?
>>>>
>>>>
>>>> /Mikael
>>>>
>>>>>
>>>>> G1CodeRootSetTest is a friend class for G1CodeRootSet so we can get
>>>>> access to its internals.
>>>>>
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8164230
>>>>> WebRev:
>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164230/webrev.00/
>>>>>
>>>>> Thank you.
>>>>>
>>>>> Regards, Kirill
>>>
>