Hi Konstantin, the fix and tests look good to me, but I think you have to wait for OK from Coleen.
One minor thing —it looks like there are no tests for other s.r.CP methods, could you please file an RFE against core-libs/j.l.reflect to cover them? Thanks, — Igor > On Dec 11, 2015, at 2:54 PM, Konstantin Shefov <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 > Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02 > > 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 >>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/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> >>>>>>> À: "Konstantin Shefov" <konstantin.she...@oracle.com> >>>>>>> Cc: "hotspot-dev developers" <hotspot-...@openjdk.java.net>, >>>>>>> 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> 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 >>>>>>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00 >>>>>>>> >>>>>>>> Thanks >>>>>>>> -Konstantin >>>>>>> >>>>> >>>> >>> >> >
