RFR JDK-8181425: Reflection API defend against issues with internal VM derived value type

[Maurizio Cimadamore]

On 22/06/17 03:21, Mandy Chung wrote:
>> On Jun 21, 2017, at 5:23 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>
>> I notice some asymmetry between the VCC and DVT cases. For VCC we have:
>>
>> * isValueCapable(Class)
>> * getValueCapableClass(Class)
>>
>> For DVT we have
>>
>> * isValueType(Class)
>> * getValueTypeClass(Class)
>> * loadValueTypeClass(Class)
>>
>> There are two kind of inconsistencies at play here - one is a plain naming issue - the suffix Class is not using consistently in all the methods.
>>
> I believe David was the author of the MinimalValueType_1_0 class and he might want to comment here.   I haven’t read up the documents in details to determine the proper terminologies.  But I have to admit that I was initially confused with isValueType that refers to DVT.
>
> I would suggest to rename 'ValueType’ to ‘DerivedValueType’ and drop the suffix ‘Class’ from *ValueType* methods and rename ValueCapable to ValueCapableClass to match DVT and VCC.  I don’t mind doing the renaming and I can follow up as a separate changeset.
>
>> The second inconsistency is that for a DVT we distinguish between get/load, while for VCC we do not. But it's a moot distinction, given that getValueTypeClass will always call loadValueTypeClass (and recheck same assertion). Maybe we should fuse those two methods?
> ValueType::arrayValueClass calls MinimalValueTypes_1_0::loadValueTypeClass to define an array of DVT.  That’s why they are separate.
>
> I expect there are lot more cleanup to be done once I get familiar with the implementation.
Ok - the changes you have now are good to go - I was just thinking loud 
looking at the method names.

Cheers
Maurizio
>
> Mandy
>
>> Cheers
>> Maurizio
>>
>>
>>
>> On 21/06/17 21:21, Mandy Chung wrote:
>>>> On Jun 21, 2017, at 9:44 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>
>>>>
>>>> On 21/06/17 16:56, Mandy Chung wrote:
>>>>> ValueType::classHasValueType is not used.  Should this be replaced with isValueCapable?  Is there a case no DVT defined for VCC (which would be the case classHasValueType would return false for VCC) but I suspect not?
>>>> I believe ValueType is supposed to be a public API, while MinimalValueType_1_0 is not. So, maybe we should keep that one as a sort of reflective predicate.
>>> I keep ValueType::classHasValueType the same behavior that loads DVT before
>>> returning.  I suspect it may not be necessesary but we can re-examine that
>>> in the future.
>>>
>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8181425/webrev.02/
>>>
>>> Changes w.r.t. webrev.01
>>> 1. Removed classHasValueType(Class<?>) and getValueTypeClass(Class<?>) which
>>> is only used by classHasValueType.
>>> 2. getValueCapableClass and getValueTypeClass no longer throws CNFE
>>>     (it throws IAE if the given type is not DVT or VCC).  MethodHandles is
>>>     updated due to the removal of this checked exception.
>>> 3. I added valhalla/mvt to jdk_lang test group that makes it handy to do verification.
>>>
>>> thanks
>>> Mandy