Hi Mandy, Since you have created a new work item to cover the test work, do let me know if you want the test code removed from the webrev for the original issue.
Also, by "nobody is meant to be changing final fields anyway", I was explaining my decision to setAccessible(true) on the final fields by default, rather than setting it via a control bit as you suggested. Seemed to save code while exposing us to minimal risk, because the field is meant to be immutable (final). Lastly, I'm sorry to see you weren't happy with the quality of my test code. Your changes seem much larger in scale, and quite different to my changes. Could you explain the benefits of your approach, vs simply adding non-static final fields to my code? No judgement, just looking to learn here. :) Best Regards Adam Farley IBM Runtimes Mandy Chung <mandy.ch...@oracle.com> wrote on 07/03/2019 15:38:48: > 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: 07/03/2019 15:40 > Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails > to throw IllegalAccessException for final fields > > Hi Adam, > > On 3/6/19 8:28 AM, Adam Farley8 wrote: > > Hi Mandy, > > > > The webrev has been updated with the new test: > > 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=jQfLigt7_Y_u9yffi19X55OptDevspJHn76z12Z_XZI&s=93WLChJClpzd43Gy4CUKWGrdi5AZcRXLyAn93S1irj8&e= > > Looks okay although I think the test adds isFinal check for the new test > case is meant to be say static final field. > > > Note that I included a handful of small improvements, and that the final > > fields are all setAccessible by default, because (a) it seemed cleaner > > than adding a new control bit, and (b) nobody is meant to be changing > > final fields anyway. > > I'm not sure what you tried to say about "nobody is meant to be changing > final fields". There should be tests covering all cases. > > In any case, since you chose to modify MethodHandlesGeneralTest, it's > okay to add tests for the static final fields which is what this fix > is about. I created a JBS issue to add test cases for the instance > final fields: > https://urldefense.proofpoint.com/v2/url? > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8220282&d=DwIC- > g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf- > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=jQfLigt7_Y_u9yffi19X55OptDevspJHn76z12Z_XZI&s=fIIn31odNd2SEKaj2zUF4qc03FJhqnQzROj09QrS0Jw&e= > > FYI. I have a patch for it and will send out for review. > https://urldefense.proofpoint.com/v2/url? > u=http-3A__cr.openjdk.java.net_-7Emchung_jdk13_webrevs_8220282_webrev. > 00_&d=DwIC-g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf- > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=jQfLigt7_Y_u9yffi19X55OptDevspJHn76z12Z_XZI&s=YhNHbETeM1kVO2frjr6aXSMgZCc69B3kDqVEiOHVSCg&e= > > 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
