On 8/28/12 8:54 AM, Daniel D. Daugherty wrote:
On 8/22/12 12:34 PM, serguei.spit...@oracle.com wrote:
Dmitry,
Thank you for the review!
Please, find new webrev version here:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2012/7191786-JVMTI-LVTT.1/
Thumbs up. No content comments.
Ok, thanks!
src/share/vm/prims/jvmtiClassFileReconstituter.hpp
No comments.
src/share/vm/prims/jvmtiClassFileReconstituter.cpp
line 178: ++attr_count;
line 421: // Write LocalVariableTable attribute
Bug fixes for the previous LVT fix? Just making sure.
Generally speaking, I do not think it is a real problem.
Just wanted to make it consistent with other attributes.
line 456: // JVMSpec| { u2 start_pc;
Breaking this line into the following would be more readable:
456 // JVMSpec| {
457 u2 start_pc;
Will do it.
Thanks!
Serguei
Dan
I also changed the line:
196 if (elem[idx].signature_cp_index > 0) {
to this:
196 if (elem[idx].signature_cp_index != 0) {
Both are correct as the signature_cp_index is u2.
But "signature_cp_index != 0" is more clear.
Thanks,
Serguei
On 8/21/12 12:45 PM, serguei.spit...@oracle.com wrote:
On 8/21/12 12:11 PM, Dmitry Samersoff wrote:
Serguei,
On 2012-08-21 23:05, serguei.spit...@oracle.com wrote:
You can see the same pattern for all attributes (for instance,
has_stackmap_table - L160).
OK.
202: local_variable_type_table_length
is always 0 if local_variable_table_length == 0
so I think if should be nested.
It does not matter.
It saves one if in some cases and makes logic better readable,
so I would like to have it changed.
Sorry!
In fact, I initially considered this option and can change it as you
prefer.
But let's wait for other reviewers input.
Thanks,
Serguei
216: It might be better to place both attr_size changes together.
I'm trying to follow the existing style.
OK
-Dmitry
Thanks,
Serguei
-Dmitry
On 2012-08-21 22:13, serguei.spit...@oracle.com wrote:
Hello,
Please, review the fix for:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7191786
Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2012/7191786-JVMTI-LVTT/
Summary:
The following CR was recently fixed:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7064927
But the same issue exists for the LocalVariableTypeTable
attribute.
The fix was tested with the modified test:
test/java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh
The modification is that a local variable with generic signature
is added
to the class DummyClassWithLVT.java:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2012/7191786-JLI-Test-For-JVMTI-LVTT/
The test patch will be integrated into the jdk8/tl after the
HotSpot fix
is promoted.
Thanks,
Serguei
