RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

Tue Dec 15 09:11:28 UTC 2015

Hi Christian

Thanks for reviewing, I have changed indents as you asked:

http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.03

-Konstantin

On 12/15/2015 06:23 AM, Christian Thalinger wrote:
>
>> On Dec 11, 2015, at 1:54 AM, Konstantin Shefov 
>> <konstantin.shefov at oracle.com <mailto:konstantin.shefov at oracle.com>> 
>> wrote:
>>
>> Hello
>>
>> Please review the new version on the patch.
>>
>> New webrev:
>> Webrev hotspot: 
>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02 
>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.02>
>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02 
>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.02>
>
> These newlines look ridiculous especially when it’s not even needed:
>
> +  // Returns a class reference index for a method or a field.
> +  public int      getClassRefIndexAt
> +                                     (int index) { return 
> getClassRefIndexAt0 (constantPoolOop, index); }
>
> Either fix the indent or just add them like regular methods should 
> look like.
>
>>
>> What has been changed:
>> 1. Added tests for the new added methods.
>> 2. Removed CP tag codes 100 - 105 from being passed to java and left 
>> only codes from the open JVM spec 
>> (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140).
>>
>> Thanks
>> -Konstantin
>>
>> On 11/27/2015 07:48 PM, Konstantin Shefov wrote:
>>> Coleen,
>>> Thanks for review
>>>
>>> On 11/24/2015 07:33 PM, Coleen Phillimore wrote:
>>>>
>>>> I have a couple preliminary comments.  First, there are no tests 
>>>> added with all this new functionality.  Tests should be added with 
>>>> the functionality changeset, not promised afterward.
>>> I will add tests.
>>>> Secondly, I do not like that JDK code knows the implementation 
>>>> details of the JVM's constant pool tags.  These should be only for 
>>>> internal use.
>>> The package "sun.reflect" is for internal use only, so it shouldn’t 
>>> be a problem.
>>>> My third comment is that I haven't looked carefully at this 
>>>> constant pool implementation but it seems very unsafe if the class 
>>>> is redefined and relies on an implementation detail in the JVM that 
>>>> can change.   I will have more comments once I look more at the 
>>>> jvmti specification.
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>> On 11/24/15 9:48 AM, Konstantin Shefov wrote:
>>>>> Hello
>>>>>
>>>>> Please, review modified webrev.
>>>>>
>>>>> I have added methods
>>>>> getNameAndTypeRefIndexAt(int index) - to get name and type entry 
>>>>> index from a method, field or indy entry index;
>>>>> getClassRefIndexAt(int index) - to get class entry index from a 
>>>>> method or field entry index;
>>>>>
>>>>> I removed previously added method
>>>>> getInvokedynamicRefInfoAt(int index) - as it is no more needed now.
>>>>>
>>>>> New webrev:
>>>>> Webrev hotspot: 
>>>>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.01 
>>>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.01>
>>>>> Webrev jdk: 
>>>>> http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.01 
>>>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.01>
>>>>>
>>>>> Thanks
>>>>> -Konstantin
>>>>>
>>>>> On 11/18/2015 02:11 PM, Konstantin Shefov wrote:
>>>>>> Remi,
>>>>>>
>>>>>> Thanks for reviewing. Your suggestion sounds sensibly.
>>>>>> May be it also has sense to make a method 
>>>>>> "getMethodRefNameAndTypeIndex(int index)" to acquire 
>>>>>> name-and-type entry index for methods also.
>>>>>>
>>>>>> -Konstantin
>>>>>>
>>>>>> On 11/18/2015 12:04 AM, Remi Forax wrote:
>>>>>>> Hi Konstantin,
>>>>>>> Technically, getInvokedynamicRefInfoAt should be 
>>>>>>> getNameAndTypeOfInvokedynamicRefInfoAt because it only extract 
>>>>>>> the nameAndType value of the InvokeDynamicRef.
>>>>>>>
>>>>>>> In term of API, I think it's better to decompose the API, i.e. 
>>>>>>> to have a method
>>>>>>>   int getInvokedynamicRefNameAndTypeIndex(int index)
>>>>>>> that returns the nameAndType index and to reuse 
>>>>>>> getNameAndTypeRefInfoAt(index) to get the corresponding array of 
>>>>>>> Strings.
>>>>>>>
>>>>>>> cheers,
>>>>>>> Rémi
>>>>>>>
>>>>>>> ----- Mail original -----
>>>>>>>> De: "Christian Thalinger" <christian.thalinger at oracle.com 
>>>>>>>> <mailto:christian.thalinger at oracle.com>>
>>>>>>>> À: "Konstantin Shefov" <konstantin.shefov at oracle.com 
>>>>>>>> <mailto:konstantin.shefov at oracle.com>>
>>>>>>>> Cc: "hotspot-dev developers" <hotspot-dev at openjdk.java.net 
>>>>>>>> <mailto:hotspot-dev at openjdk.java.net>>, 
>>>>>>>> core-libs-dev at openjdk.java.net 
>>>>>>>> <mailto:core-libs-dev at openjdk.java.net>
>>>>>>>> Envoyé: Lundi 16 Novembre 2015 23:41:45
>>>>>>>> Objet: Re: RFR [9] 8141615: Add new public methods to 
>>>>>>>> sun.reflect.ConstantPool
>>>>>>>>
>>>>>>>> [CC'ing core-libs-dev]
>>>>>>>>
>>>>>>>>> On Nov 13, 2015, at 4:55 AM, Konstantin Shefov
>>>>>>>>> <konstantin.shefov at oracle.com 
>>>>>>>>> <mailto:konstantin.shefov at oracle.com>> wrote:
>>>>>>>>>
>>>>>>>>> Hello
>>>>>>>>>
>>>>>>>>> Please review an enhancement to add three new methods to
>>>>>>>>> sun.reflect.ConstantPool class.
>>>>>>>>> The following methods are suggested:
>>>>>>>>>
>>>>>>>>> public String[] getNameAndTypeRefInfoAt(int index) - returns 
>>>>>>>>> string
>>>>>>>>> representation of name and type from a NameAndType constant 
>>>>>>>>> pool entry
>>>>>>>>> with the specified index
>>>>>>>>>
>>>>>>>>> public String[] getInvokedynamicRefInfoAt(int index) - returns 
>>>>>>>>> string
>>>>>>>>> representation of name and type from an InvokeDynamic constant 
>>>>>>>>> pool entry
>>>>>>>>> with the specified index
>>>>>>>>>
>>>>>>>>> public Tag getTagAt(int index) - returns a Tag enum instance 
>>>>>>>>> of a constant
>>>>>>>>> pool entry with the specified index
>>>>>>>>>
>>>>>>>>> These three methods could be used for testing API working with 
>>>>>>>>> constant
>>>>>>>>> pool, e.g. JVMCI.
>>>>>>>>>
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8141615
>>>>>>>>> Webrev hotspot:
>>>>>>>>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.00 
>>>>>>>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.00>
>>>>>>>>> Webrev jdk: 
>>>>>>>>> http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> -Konstantin
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>