Hi Coleen,
Thank you a lot for the review!
On 1/24/13 3:55 PM, Coleen Phillimore wrote:
Hi Serguei,
Putting functions in alphabetical order seems silly, it's better to
have utility functions directly above (or below) the function that
calls them. I'd take out the comment.
I have started looking at this code a bit. This function
find_or_append_indirect_entry() should be used for the other indirect
entries also, shouldn't it? The code looks familiar to the inside of
the case statement to FieldRef, InterfaceRef and MethodRef.
You've got it right.
Yes, I noticed it but did not want to mess with it until it is proven to
work well.
My plan was to fix it for the FieldRef, InterfaceRef and MethodRef in a
next round of fixes.
Also is boot_spec an indirect index too that has to be appended?
564 int boot_spec =
scratch_cp->invoke_dynamic_bootstrap_method_ref_index_at(scratch_i);
This is a nice catch, will fix it.
I thought, it is an index into the operands array, but it refers to a
CONSTANT_MethodHandle element.
Thanks,
Serguei
Thanks,
Coleen
On 01/24/2013 05:56 PM, serguei.spit...@oracle.com wrote:
Christian,
Thank you a lot for the review!
Nice catch with the ordering.
In fact, I tried to follow the order but missed that the
find_new_index should go first. :)
Thanks,
Serguei
On 1/24/13 2:14 PM, Christian Thalinger wrote:
On Jan 22, 2013, at 4:07 PM, serguei.spit...@oracle.com wrote:
Please, review the fix for:
https://jbs.oracle.com/bugs/browse/JDK-8006542
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.1/
src/share/vm/prims/jvmtiRedefineClasses.hpp:
+ // Support for constant pool merging (these routines are in alpha
order):
void append_entry(constantPoolHandle scratch_cp, int scratch_i,
constantPoolHandle *merge_cp_p, int *merge_cp_length_p, TRAPS);
+ int find_or_append_indirect_entry(constantPoolHandle scratch_cp,
int scratch_i,
+ constantPoolHandle *merge_cp_p, int *merge_cp_length_p, TRAPS);
int find_new_index(int old_index);
Not really alpha order ;-)
Otherwise this looks good (as far as I can tell).
-- Chris
Summary:
Need a support for invokedynamic entry kinds when new and old
constant pools are merged.
Testing:
vm/mlvm/indy/func/jvmti/redefineClassInBootstrap
Asked the VM SQE team to develop new INDY tests for better coverage.
Thanks,
Serguei
