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.
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);
Thanks,
Coleen
On 01/24/2013 05:56 PM, [email protected] 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, [email protected] 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
