> On Dec 14, 2015, at 11:11 PM, Konstantin Shefov > <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/~kshefov/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 <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 <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>https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140 >>> >>> <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" < >>>>>>>>> <mailto:konstantin.she...@oracle.com>konstantin.she...@oracle.com >>>>>>>>> <mailto:konstantin.she...@oracle.com>> >>>>>>>>> Cc: "hotspot-dev developers" < >>>>>>>>> <mailto:hotspot-...@openjdk.java.net>hotspot-...@openjdk.java.net >>>>>>>>> <mailto: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 >>>>>>>>>> <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 >>>>>>>>>> <http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00> >>>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>>> -Konstantin >>>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >