RFR (S) 8214822: Move ConcurrentHashTable VALUE parameter to CONFIG

[David Holmes]

That looks good to me!

Thanks,
David

On 21/06/2019 8:18 am, coleen.phillimore at oracle.com wrote:
> 
> Yes, this is nicer.  Thank you for the suggestion.
> http://cr.openjdk.java.net/~coleenp/2019/8214822.02/webrev
> 
> Coleen
> 
> On 6/21/19 10:28 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 6/21/19 10:12 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 21/06/2019 5:40 am, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> On 6/20/19 9:29 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 20/06/2019 10:39 am, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: make VALUE parameter be included in CONFIG configuration, 
>>>>>> also remove BaseConfig
>>>>>>
>>>>>> Tested with tier1-3.
>>>>>>
>>>>>> open webrev at 
>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8214822.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8214822
>>>>>
>>>>> I don't understand all the "value" typedef's you have introduced:
>>>>>
>>>>> +  typedef WeakHandle<vm_string_table_data> value;
>>>>> +  typedef Symbol* value;  // value of the Node in the hashtable
>>>>> +  typedef WeakHandle<vm_resolved_method_table_data> value;
>>>>>
>>>>> If "value" is meant to be a type then it should be "Value", as 
>>>>> "value" is used as the variable name everywhere. But I don't see 
>>>>> you actually using the typedefs anywhere ??
>>>>
>>>> Here it is:
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/2019/8214822.01/webrev/src/hotspot/share/utilities/concurrentHashTable.hpp.frames.html 
>>>>
>>>>
>>>> 43 typedef typename CONFIG::value VALUE;
>>>>
>>>> I can capitalize it.  I left it VALUE vs some other name to reduce 
>>>> the changes, and capital VALUE I think helps the reader know that 
>>>> this is like a parameterized type (from the CONFIG).
>>>
>>> I see two sources of confusion in the current code. First "value" in 
>>> the typedef appears to conflict with "value" as a variable name. It 
>>> may not actually conflict due to the way C++ interprets the name in 
>>> context but it looks very strange to me to have e.g.:
>>>
>>> typedef WeakHandle<vm_resolved_method_table_data> value;
>>>
>>> static uintx get_hash(WeakHandle<vm_resolved_method_table_data> 
>>> const& value,
>>>                          bool* is_dead) {
>>>
>>> So I'd prefer to see:
>>>
>>> typedef WeakHandle<vm_resolved_method_table_data> Value;
>>
>> Okay, the compiler doesn't have a problem with it.   It appears that 
>> the parameter name hides the typedef otherwise it wouldn't compile, 
>> but there's no warning message about that.
>>
>>>
>>> Second now that you have the typedef why aren't you using it in the 
>>> function declaration ie.
>>>
>>> static uintx get_hash(Value const& value, bool* is_dead) {
>>>
>>
>> I could change this too.
>>
>> Thanks,
>> Coleen
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>
>