On 3/11/19 8:48 AM, Adam Farley8 wrote:
Hi Mandy,
Thank you for explaining. :)
Unfortunately I'm only a mere Author, and I cannot submit test runs on
the shared test server.
I have run the test locally, and it passes against a patched build and
fails correctly agaionst an unpatched build, so I think we're good to go.
I will sponsor this patch for you when CSR is approved.
Can you tell me if there's something I can do to help move the CSR forward?
No additional action from your end. It's already in finalized state.
Also, can you tell me if an additional CSR would need to be created for
other releases if this fix gets backported?
Which release are you thinking to backport to?
IMO I don't think this fix is critical for existing releases
and this fix has behavioral change.
Mandy
Best Regards
Adam Farley
IBM Runtimes
Mandy Chung <mandy.ch...@oracle.com> wrote on 07/03/2019 23:17:15:
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 23:19
Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails
to throw IllegalAccessException for final fields
On 3/7/19 9:44 AM, Adam Farley8 wrote:
> 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.
You push what you have. Your fix should come with a regression test.
Please make sure you do jdk-submit to verify the test work.
> 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?
First, I dont see my test update is large scale. Second, it's not
about the quality of the test code. The size of the change is not
the sole factor to consider but that seems to bother you. Anyway,
the test itself isn't easy to work with. You have done the fix to
include the regression test which is okay.
Your patch made me to look at the test closer which I started from
the version in the repo, not based on your version. Once you push
your fix, I can generate a new webrev to see the diff that I can
revise on top of your version.
I change a couple things. The test uses Error in HasFields.CASES to
indicate a negative test. For a final field, it has no write access
and it is a negative test case except unreflectSetter on an instance
final field whose accessible flag is true. That explains why
Error.class is in the third element of the new case added for final
field that actually reflects if a test case is positive or negative
when calling testAccessor. What you added is treating setter on
final field is positive test case and then returns when IAE is thrown.
It does the work but passing incorrect information.
I uncovered some unnecessary test cases for findSetter on
static fields and findStaticSetter on non-static fields. It's an
existing test issue and I think they should be filtered out and
that's why CASES is separated into two separate ArrayLists and the new
static cases(int testMode) method. I see that you used a new kind for
static final field that may be a good approach. When I pull down your
change, I will send out a revision (that's why I wait for your patch
to go in first and see any thing I should change).
Hope this helps.
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
