[core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[David Holmes]

Sorry Remi I overlooked responding to this. Thanks for confirming I 
don't have to do anything at this time.

David

On 17/05/2018 8:02 PM, forax at univ-mlv.fr wrote:
> 
> 
> ----- Mail original -----
>> De: "David Holmes" <david.holmes at oracle.com>
>> À: "Remi Forax" <forax at univ-mlv.fr>, "Alan Bateman" <Alan.Bateman at oracle.com>
>> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>> Envoyé: Jeudi 17 Mai 2018 10:37:46
>> Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
> 
>> Hi Remi,
> 
> Hi David
> 
>>
>> On 17/05/2018 6:16 PM, Remi Forax wrote:
>>> Hi all,
>>>
>>> ----- Mail original -----
>>>> De: "Alan Bateman" <Alan.Bateman at oracle.com>
>>>> À: "David Holmes" <david.holmes at oracle.com>, "core-libs-dev"
>>>> <core-libs-dev at openjdk.java.net>
>>>> Envoyé: Mardi 15 Mai 2018 15:53:44
>>>> Objet: Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based
>>>> Access Control
>>>
>>>> On 15/05/2018 01:52, David Holmes wrote:
>>>>> This review is being spread across four groups: langtools, core-libs,
>>>>> hotspot and serviceability. This is the specific review thread for
>>>>> core-libs - webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
>>>
>>> [...]
>>>
>>>> Maybe a question for Kumar but are we planning to pull in any ASM
>>>> updates for JDK 11? NestMembers extends Attribute looks okay, I'm less
>>>> sure about the change to ClassReader as I don't know if there is
>>>> somewhere else in ASM that has the list of attributes to always parse.
>>>
>>> With my ASM hat,
>>> the current master of ASM (the release of ASM 6.2 is scheduled for the next
>>> week-end) already supports nestmates (and constant dynamic and preview feature)
>>> so i suppose that at some point in the future Kumar will merge it to the JDK.
>>
>> Unfortunately Kumar is no longer with us.
> 
> I will miss him.
> 
>>
>>> We have recently changed the way we implement features in ASM, instead of having
>>> features lingering in different branches, we now integrate them directly in the
>>> master under an experimental flag (ASM7_EXPERIMENTAL), which means for the JDK
>>> that it is no longer necessary to wait until the release of ASM 7 because it
>>> can use the experimental support of ASM 6.2.
>>> (note that experimental doesn't mean full of bugs, or half baked or anything
>>> like this, it means that the feature is not yet integrated in a released JDK).
>>>
>>> I've taking a look to the code in this patch, i've two comments,
>>> - in Attributes, it seems that the code store the bytecode slice corresponding
>>> to the attribute only to use its length as argument of the ByteVector which is
>>> like an ArrayList of byte, it grows automatically so the initial capacity is a
>>> perf optimization. Perhaps the byte array is used somewhere else ?
>>> - patching the ClassReader.accept is really a quick hack because the method
>>> accept with 3 arguments is not patched so if this method is called somewhere in
>>> the JDK it will behave as it should.
>>
>> I'll take a look at this. To be honest I don't even remember who
>> provided those changes ... I thought you had provided feedback at some
>> point in the past :)
> 
> yes, i may have, i do not remember.
> 
>> There's a valhalla-dev email with a link that's no
>> longer valid:
>>
>> https://gitlab.ow2.org/asm/asm/tree/NEST_MATES
> 
> as i said above, the support has been integrated in the master of ASM,
> see https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/ClassVisitor.java#L156
> and https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/ClassVisitor.java#L246
>   
> the current code is fine, just not optimal, but given that when the JDK will use ASM 6.2, this code will have to be removed, i do not think you should spend some time one this.
> 
>>
>> Thanks,
>> David
> 
> regards,
> Rémi
>