----- Mail original ----- > De: "David Holmes" <david.hol...@oracle.com> > À: "Remi Forax" <fo...@univ-mlv.fr>, "Alan Bateman" <alan.bate...@oracle.com> > Cc: "core-libs-dev" <core-libs-dev@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
Advertising
> Hi Remi, Hi David > > On 17/05/2018 6:16 PM, Remi Forax wrote: >> Hi all, >> >> ----- Mail original ----- >>> De: "Alan Bateman" <alan.bate...@oracle.com> >>> À: "David Holmes" <david.hol...@oracle.com>, "core-libs-dev" >>> <core-libs-dev@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
