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

[coleen.phillimore at oracle.com]

Thanks, David!
Coleen

On 6/21/19 12:02 PM, David Holmes wrote:
> 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
>>>>>
>>>
>>