Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[serguei.spitsyn at oracle.com]

On 6/23/16 04:40, David Holmes wrote:
>
>
> On 23/06/2016 9:33 PM, serguei.spitsyn at oracle.com wrote:
>> On 6/23/16 04:27, David Holmes wrote:
>>> On 23/06/2016 9:08 PM, serguei.spitsyn at oracle.com wrote:
>>>> On 6/23/16 03:51, David Holmes wrote:
>>>>> On 23/06/2016 6:04 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> On 6/23/16 00:51, Alan Bateman wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 23/06/2016 00:20, serguei.spitsyn at oracle.com wrote:
>>>>>>>> :
>>>>>>>>
>>>>>>>> I agree with it.
>>>>>>>> Thank you for pointing to this JVMTI example.
>>>>>>>> I did not find in the JNI where the names are checked to be legal.
>>>>>>>> We are going to open a can of worms with this kind of check as 
>>>>>>>> there
>>>>>>>> can be many corner cases to cover.
>>>>>>> The primary use-case for this function is from within the CLFH
>>>>>>> callback so have it return the unnamed module when calling with
>>>>>>> garage
>>>>>>> is probably okay, it just needs to be specified.
>>>>>>>
>>>>>>> Will you update the webrev to reflect where we've got to this in 
>>>>>>> this
>>>>>>> discussion?
>>>>>>
>>>>>> I need to undo my fix for additional check.
>>>>>> My up-to-date understanding is that we have to explicitly specify 
>>>>>> two
>>>>>> points:
>>>>>>   - the function returns the unnamed module if the package does not
>>>>>> exist
>>>>>>   - the function does not check the package names for validness and
>>>>>> always returns the unnamed module for illegal package names
>>>>>>
>>>>>> It is better to specify these points explicitly to avoid possible
>>>>>> confusion.
>>>>>
>>>>> I don't agree - nothing else refers to invalid "names" in JVM TI. I
>>>>> don't see any need to call this out here. If the name supplied is not
>>>>> a legal package name then it will not match - end of story.
>>>>
>>>> David,
>>>>
>>>> It was the original approach that is in the CCC.
>>>> You were the one who got confused here, and it convinced me that this
>>>> needs to be more clear.
>>>> All these changes are because you started the discussion. :)
>>>> It was useful anyway.
>>>
>>> I never said anything about illegal package names.
>>
>> True.
>> This is my (probably, wrong) attempt to make it more clear.
>>
>>>
>>>> Are you against both clarifications or just the last one?
>>>> I'm Ok to return it back as it is in the CCC.
>>>> It will save time on the CCC approval.
>>>>
>>>> This is the last addition to the spec:
>>>>
>>>> + The unnamed module is returned if the specified package does not
>>>> exist.
>>>
>>> The notion of "package does not exist" is ill-defined. This case is
>>> already covered by the primary specification.
>>>
>>>> + The function does not check if the specified package name is 
>>>> illegal.
>>>
>>> This does not need to be stated as it is not stated anywhere else for
>>> anything else that may have legal and illegal forms.
>>
>> Good.
>> This is a relevant fragment from current version of CCC:
>>
>> +      <description>
>> +        Return the <code>java.lang.reflect.Module</code> object for a
>> module
>> +        defined to a class loader that contains a given package.
>> +        The module is returned via <code>module_ptr</code>.
>> +        <p/>
>> +        If a named module is defined to the class loader and it
>> +        contains the package then that named module is returned,
>> +        otherwise the unnamed module of the class loader is returned.
>> +        If the package name is the empty string then this function
>> +        always returns the unnamed module for the class loader.
>> +        <p/>
>
> As Stanislav said explicitly mentioning the empty string is not really 
> necessary - but I don't see it as harmful.

Me too.
Stanislav did not insist on this matter.

>
>> Does it looks Ok?
>
> Yes - as good now as it was in the CCC discussion :)

Great!
I'm also waiting for a feedback from Alan.


Thanks!
Serguei

>
> Cheers,
> David
>