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
