> On Dec 11, 2015, at 1:54 AM, 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 > <http://cr.openjdk.java.net/~kshefov/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 >>>> 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 >>>>>>> >>>>> >>>> >>> >> >