Hi Mandy, The webrev has been updated with the new test: http://cr.openjdk.java.net/~afarley/8216558/webrev/
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. Open for review. Best Regards Adam Farley IBM Runtimes Mandy Chung <mandy.ch...@oracle.com> wrote on 01/03/2019 17:50:49: > 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: 01/03/2019 17:50 > Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails > to throw IllegalAccessException for final fields > > Hi Adam, > > You can add a new test for this specific test case. > MethodHandlesGeneralTest is a general test that you either update > it to fit in existing testing framework or add a new test which > will be simpler for you. Please include the test in the webrev > for easier review. > > Mandy > > On 3/1/19 2:31 AM, Adam Farley8 wrote: > > Hi Mandy, > > > > Historically, the bigger the change I propose, the more months it takes > > the OpenJDK community to approve. > > > > I'm not sure I can justify adding an entire class to test a very > > specific case. > > > > Additionally, HasFields seems excessively complex. I don't understand > > why it feels the > > need to spend 34 lines of code parsing, processing, and storing 20 > > minimal variables it already has. > > > > Seems like an informative comment, followed by a littany of > > "cases.add..." would have been a simpler choice. > > > > Could you tell me if I'm missing something? > > > > Best Regards > > > > Adam Farley > > IBM Runtimes > > > > > > Mandy Chung <mandy.ch...@oracle.com> wrote on 21/02/2019 17:37:54: > > > >> 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: 21/02/2019 17:41 > >> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails > >> to throw IllegalAccessException for final fields > >> > >> Hi Adam, > >> > >> On 2/14/19 3:16 AM, Adam Farley8 wrote: > >> > Hi Mandy, > >> > > >> > Apologies for the delay. > >> > >> Same here as I returned from vacation yesterday. > >> > >> > Could you review this cdiff as a proposal for the jtreg test? > >> > > >> > Made sense to modify the existing test set for MethodHandle rather than > >> > add a new one. > >> > >> Yes it'd be better if you extend the MethodHandlesGeneralTest and > >> MethodHandlesTest to test the write access. > >> > >> To add the test cases properly, MethodHandlesTest.HasFields > >> should be modified to include the read-only fields. It might > >> be cleaner to add a new HasReadOnlyFields class and maybe a new > >> TEST_SET_ACCESSIBLE bit that requests to call setAccessible(true) > >> and testSetter expects unreflectSetter to throw exception on > >> static final fields (possibly additional bits might be needed). > >> > >> 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 > 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
