RFR(S): 8164230: Convert TestCodeCacheRemSet_test to GTest

[Kirill Zhaldybin]

Mikael,

Thank you for review!

Regards, Kirill

On 09/01/2016 11:04 AM, Mikael Gerdin wrote:
> 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
>>>>
>>