[8] Review request for 8004970, 8004971, and 8006817: implement serialization in lambda metafactory and metafactory fix

[Robert Field]

On 02/15/13 15:40, Remi Forax wrote:
> On 02/12/2013 04:23 PM, Robert Field wrote:
>> Please review the fixes for CRs:
>>
>>          http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004970
>>
>>          http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004971
>>
>>          http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8006817
>>
>>
>>
>> Webrev:
>>
>>          http://cr.openjdk.java.net/~rfield/8004970
>>
>> Thank,
>> Robert
>>
>>
> Hi Robert,
> few comments,

Thanks much, Rémi, for the review.

>
> in MethodHandleInfo:
>    - getReferenceKindString should not be public, it's a detail of
> implementation.

Query to the JSR-292 lead in process.

>
> in InnerClassLambdaMetaFactory:
>    - spinInnerClass() should return a simple Class<?>.

Yep.

>    - generateWriteReplace: there is still a mv.dup() that should be
> replaced by
> mv.visitInsn(DUP);

Yep.

>
> in AbstractValidatingLambdaMetafactory:
>     - field markerInterfaces should be declared as Class<?>[]

Yep
>       (you can replace all Class by Class<?>)

Will scan.

>
> in SerializedLambda:
>     - I think it should declare a serialUID, so you don't need a ad-hoc
> versioning.
>       (to answer to the question line 235)

Will do.

>     - Also does the constructor
> SerializedLambda(Class ,MethodHandle,MethodHandle,MethodType,Object[])
>       still used ?

Nope.  It's history.

>
> in TypeConvertingMethodAdapter:
>     - at the end, load/dup/areturn/getfield should be replaced by their
> corresponding
>       code inlined. I don't see a real interest to these too small method.
>       Only iconst is really complex so worth a dedicated method.

I find that it makes the code more readable, but there are too few uses 
to justify, and I will defer to you on this score.

Thanks again,
Robert

>
> cheers,
> Rémi
>
>
>
>