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

[coleen.phillimore at oracle.com]

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
>>>
>