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

[David Holmes]

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;

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

Thanks,
David

> Thanks,
> Coleen
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks,
>>> Coleen
>