RFR(M): 8057043: Type annotations not retained during class redefine / retransform
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Oct 15 13:47:52 UTC 2014
On 10/15/14 7:34 AM, Coleen Phillimore wrote:
>
> There are lots of other rewrite_cp_refs_in* function calls. Please
> indent your function like them, not differently.
The above implies that my answer below was made without sufficient
context... my apologies for that.
The general rule is to follow the existing style in the file so
if there are rewrite_cp_refs_in* function calls in the file, then
please follow that style. Unless, of course, you want to fix all
of them to follow the HotSpot style guideline:
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
> Use good taste to break lines and align corresponding tokens
> on adjacent lines.
but that may cause Coleen some heartburn :-)
Dan
>
> Coleen
>
> On 10/15/14, 9:05 AM, Daniel D. Daugherty wrote:
>> On 10/15/14 5:22 AM, Andreas Eriksson wrote:
>>> Thanks Serguei.
>>>
>>> I have a question about the if-blocks that had the wrong indent:
>>>
>>> 2335 if
>>> (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
>>>
>>> 2336 byte_i, "method_info", THREAD)) {
>>>
>>> How should I indent them?
>>
>> Trying again without the line numbers...
>>
>> if
>> (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
>> byte_i,
>> "method_info",
>> THREAD)) {
>>
>> Just in case, TB messes with the spacing again, the "byte_i" line and
>> "THREAD" lines are aligned under "method_type_annotations".
>>
>> Dan
>>
>>
>>>
>>> /Andreas
>>>
>>> On 2014-10-15 07:00, serguei.spitsyn at oracle.com wrote:
>>>> Hi Andreas,
>>>>
>>>> Sorry I did not reply on this early.
>>>> I assumed, it is a thumbs up from me.
>>>> Just wanted make it clean now. :)
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 10/13/14 3:09 AM, Andreas Eriksson wrote:
>>>>> Hi Serguei, thanks for looking at this!
>>>>>
>>>>> I'll make sure to fix the style problems.
>>>>> For the symbolic names / #defines, please see my answer to Coleen.
>>>>>
>>>>> Regards,
>>>>> Andreas
>>>>>
>>>>> On 2014-10-11 12:37, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Andreas,
>>>>>>
>>>>>>
>>>>>> Thank you for fixing this issue!
>>>>>> The fix looks nice, I do not see any logical issues.
>>>>>>
>>>>>>
>>>>>> Only minor comments...
>>>>>>
>>>>>>
>>>>>> src/share/vm/prims/jvmtiRedefineClasses.cpp
>>>>>>
>>>>>> 2281 } // end rewrite_cp_refs_in_class_type_annotations(
>>>>>> 2315 } // end rewrite_cp_refs_in_fields_type_annotations(
>>>>>> 2345 } // end rewrite_cp_refs_in_methods_type_annotations()
>>>>>> 2397 } // end rewrite_cp_refs_in_type_annotations_typeArray
>>>>>> 2443 } // end rewrite_cp_refs_in_type_annotation_struct
>>>>>> 2785 } // end skip_type_annotation_target
>>>>>> 2844 } // end skip_type_annotation_type_path
>>>>>>
>>>>>> The ')' is missed at 2281, 2315.
>>>>>> The 2397-2844 are inconsistent with the 2345 and other
>>>>>> function-end comments in the file.
>>>>>>
>>>>>>
>>>>>> 2335 if
>>>>>> (!rewrite_cp_refs_in_type_annotations_typeArray(method_type_annotations,
>>>>>>
>>>>>> 2336 byte_i, "method_info", THREAD)) {
>>>>>> . . .
>>>>>> 2378 if
>>>>>> (!rewrite_cp_refs_in_type_annotation_struct(type_annotations_typeArray,
>>>>>>
>>>>>> 2379 byte_i_ref, location_mesg, THREAD)) {
>>>>>> . . .
>>>>>> 2427 if (!skip_type_annotation_target(type_annotations_typeArray,
>>>>>> 2428 byte_i_ref, location_mesg, THREAD)) {
>>>>>> 2429 return false;
>>>>>> 2430 }
>>>>>> 2431
>>>>>> 2432 if
>>>>>> (!skip_type_annotation_type_path(type_annotations_typeArray,
>>>>>> 2433 byte_i_ref, THREAD)) {
>>>>>> 2434 return false;
>>>>>> 2435 }
>>>>>> 2436
>>>>>> 2437 if
>>>>>> (!rewrite_cp_refs_in_annotation_struct(type_annotations_typeArray,
>>>>>> 2438 byte_i_ref, THREAD)) {
>>>>>> 2439 return false;
>>>>>> Wrong indent at 2336, 2379, etc.
>>>>>>
>>>>>>
>>>>>> I also concur with Coleen that it would be good to define and use
>>>>>> symbolic names for the hexa-decimal constants used in the fix.
>>>>>>
>>>>>>
>>>>>>
>>>>>> test/runtime/RedefineTests/RedefineAnnotations.java
>>>>>>
>>>>>> Java indent must be 4, not 2.
>>>>>>
>>>>>> 253 @TestAnn(site="returnTypeAnnotation") Class
>>>>>> typeAnnotatedMethod(@TestAnn(site="formalParameterTypeAnnotation") TypeAnnotatedTestClass
>>>>>> arg)
>>>>>>
>>>>>> The line is too long.
>>>>>>
>>>>>>
>>>>>> 143 }
>>>>>> 144 public static void main(String argv[]) {
>>>>>> . . .
>>>>>> 209 }
>>>>>> 210 private static void checkAnnotations(AnnotatedType p) {
>>>>>> 211 checkAnnotations(p.getAnnotations());
>>>>>> 212 }
>>>>>> 213 private static void checkAnnotations(AnnotatedType[]
>>>>>> annoTypes) {
>>>>>> 214 for (AnnotatedType p : annoTypes)
>>>>>> checkAnnotations(p.getAnnotations());
>>>>>> 215 }
>>>>>> 216 private static void
>>>>>> checkAnnotations(Class<TypeAnnotatedTestClass> c) {
>>>>>> . . .
>>>>>> 257 }
>>>>>> 258 public void run() {}
>>>>>>
>>>>>> Adding empty lines between method definitions would improve
>>>>>> readability.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 10/9/14 6:21 AM, Andreas Eriksson wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review this patch to RedefineClasses to allow type
>>>>>>> annotations to be preserved.
>>>>>>>
>>>>>>> Summary:
>>>>>>> During redefine / retransform class the constant pool indexes
>>>>>>> can change.
>>>>>>> Since annotations have indexes into the constant pool these
>>>>>>> indexes need to be rewritten.
>>>>>>> This is already done for regular annotations, but not for type
>>>>>>> annotations.
>>>>>>> This patch adds code to add this rewriting for the type
>>>>>>> annotations as well.
>>>>>>> The patch also contains minor changes to ClassFileReconstituter,
>>>>>>> to make sure that type annotations are preserved during a
>>>>>>> redefine / retransform class operation.
>>>>>>> It also has a test that uses asm to change constant pool indexes
>>>>>>> through a retransform, and then verifies that type annotations
>>>>>>> are preserved.
>>>>>>>
>>>>>>> Detail:
>>>>>>> A type annotation struct consists of some target information and
>>>>>>> a type path, followed by a regular annotation struct.
>>>>>>> Constant pool indexes are only present in the regular annotation
>>>>>>> struct.
>>>>>>> The added code skips over the type annotation specific parts,
>>>>>>> then calls previously existing code to rewrite constant pool
>>>>>>> indexes in the regular annotation struct.
>>>>>>> Please see the Java SE 8 Ed. VM Spec. section 4.7.20 for more
>>>>>>> info about the type annotation struct.
>>>>>>>
>>>>>>> JPRT with the new test passes without failures on all platforms.
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~aeriksso/8057043/webrev.00/
>>>>>>>
>>>>>>> Bug:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8057043
>>>>>>>
>>>>>>> Regards
>>>>>>> Andreas
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list