Looks good. Thanks.
> On Dec 16, 2015, at 1:13 AM, Konstantin Shefov <konstantin.she...@oracle.com>
> wrote:
>
> Christian
>
> I have fixed the enum so it uses "ENUMENTRY(int)" format now and does linear
> search.
> http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04/
>
> -Konstantin
>
> On 12/15/2015 08:36 PM, Christian Thalinger wrote:
>>
>>> On Dec 14, 2015, at 11:11 PM, Konstantin Shefov
>>> <konstantin.she...@oracle.com <mailto:konstantin.she...@oracle.com>> wrote:
>>>
>>> Hi Christian
>>>
>>> Thanks for reviewing, I have changed indents as you asked:
>>>
>>> http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.03
>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.03>
>>
>> Thanks. I’m still not comfortable with the enum. It would be great if we
>> could get the values from the VM like in JVMCI:
>>
>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java#l101
>>
>> but that would be overkill here. But I would like to see the enum entries
>> take the integer values as arguments, like here:
>>
>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.sparc/src/jdk/vm/ci/sparc/SPARCKind.java#l27
>>
>> and either do a simple linear search to find the entry or build up a table
>> like the HotSpotConstantPool example above.
>>
>>>
>>> -Konstantin
>>>
>>> On 12/15/2015 06:23 AM, Christian Thalinger wrote:
>>>>
>>>>> On Dec 11, 2015, at 1:54 AM, Konstantin Shefov
>>>>> <konstantin.she...@oracle.com <mailto:konstantin.she...@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.thalin...@oracle.com
>>>>>>>>>>> <mailto:christian.thalin...@oracle.com>>
>>>>>>>>>>> À: "Konstantin Shefov" <konstantin.she...@oracle.com>
>>>>>>>>>>> Cc: "hotspot-dev developers" <hotspot-...@openjdk.java.net>,
>>>>>>>>>>> core-libs-dev@openjdk.java.net
>>>>>>>>>>> <mailto:core-libs-dev@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.she...@oracle.com
>>>>>>>>>>>> <mailto:konstantin.she...@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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
