Hi Mandy, I've made the changes to the webrev and uploaded the corrected version.
I've also uploaded the zips in the csr and bug. As for the test, I have mentioned that the test is attached to the bug. I attached the same test to the CSR. Are you saying that the test supplied is not a suitable test? Please advise. Best Regards Adam Farley IBM Runtimes Mandy Chung <mandy.ch...@oracle.com> wrote on 30/01/2019 18:22:40: > From: Mandy Chung <mandy.ch...@oracle.com> > To: Adam Farley8 <adam.far...@uk.ibm.com> > Cc: core-libs-dev <core-libs-dev@openjdk.java.net> > Date: 30/01/2019 18:22 > Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails > to throw IllegalAccessException for final fields > > Hi Adam, > > On 1/30/19 7:52 AM, Adam Farley8 wrote: > > Hi Mandy, > > > > CSR has been raised: https://urldefense.proofpoint.com/v2/url? > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8218061&d=DwIC- > g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf- > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=wVe5GwdYZTp4EVs77yXBcAfCyKsPsjBCVBjF0XjirEs&s=1DFz7ShyytN1Bj8a3YtRyTpwbT1Cn1o- > Jf-ec5OvF7w&e= > > Thanks for doing it. A couple comments: > > I think the compatibility risk should be low rather than minimal. > Even the code shouldn't be doing that, unreflectSetter does > allow to set a static final field in the current implementation. > It's low because Field::set throws exception if it's a static > final field and so it's expected that very few existing libraries > would use unreflectSetter. > > FWIW. There are few existing libraries hacking the reflection > to change their static final fields but very few. > > Since the spec diff is small, I suggest to cut-n-paste the diff > on the javadoc to the specification section and easier to see > the spec change. webrev is not generally needed for CSR but > welcome to include that if it helps the review. > > I made some minor edit to the CSR to make the problem and solution > clearer. > > > The webrev I used includes John's comments, your additions, the > > one-line-IF, > > and I also took the liberty of moving the unreflectField method underneath > > the unreflectSetter method, because it seemed strange that it was located > > between unreflectSetter and unreflectGetter. > > > > Online webrev has been updated to include these changes: > > https://urldefense.proofpoint.com/v2/url? > u=http-3A__cr.openjdk.java.net_-7Eafarley_8216558_webrev&d=DwIC- > g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf- > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=wVe5GwdYZTp4EVs77yXBcAfCyKsPsjBCVBjF0XjirEs&s=0KYjj1f6lxz1DSEcK1UnPJ3ozG9- > VwiUx2-B_u9EaD0&e= > > Looks good but the patch still needs a regression test. Please > include the regression test for code review. > > Formatting nit: a space is missing between 'if' and '(' and > plase use the 4-space identation of the throw. > > 1901 if(isSetter && field.isStatic() && field.isFinal()) > 1902 throw field.makeAccessException("static final > field has no write access", this); > > Mandy > Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
