This seems fine. Only one minor comment (where I don't need to see
another webrev)
+ constantPoolHandle cp = constantPoolHandle(THREAD,
sun_reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));
would be shorter as
+ constantPoolHandle cp(THREAD,
sun_reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));
and avoid implicit copy construction.
Thanks for adding tests.
Coleen
On 1/11/16 8:34 AM, Konstantin Shefov wrote:
Kindly reminder.
Already approved by C. Thalinger and I. Ignatyev.
Thanks
-Konstantin
On 12/17/2015 11:26 AM, Konstantin Shefov wrote:
Hi Coleen
You have previously reviewed this enhancement and made a few comments
I have resolved them, so could you look at the webrevs again, please?
I have added tests, removed cp tags that are not in JVM spec (100 -
105) and made some other changes.
JDK: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04
HOTSPOT: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02
Thanks
-Konstantin
On 12/16/2015 07:42 PM, Christian Thalinger wrote:
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