Serguei, looks good!
/R On Aug 22, 2012, at 8: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/ > > 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 >>> >> >
