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

[coleen.phillimore at oracle.com]

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