RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails to throw IllegalAccessException for final fields

[Adam Farley8]

Hi Mandy,


Mandy Chung <mandy.chung at oracle.com> wrote on 22/03/2019 16:56:12:

> From: Mandy Chung <mandy.chung at oracle.com>
> To: Joe Darcy <joe.darcy at oracle.com>, Adam Farley8 
<adam.farley at uk.ibm.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>
> Date: 22/03/2019 16:58
> Subject: Re: RFR: JDK-8216558: Lookup.unreflectSetter(Field) fails 
> to throw IllegalAccessException for final fields
> 
> Hi Adam,

> On 3/22/19 8:40 AM, Joe Darcy wrote:
> 
> Please update distinct versions of a webrev (e.g. distinguished with
> .1, .2 directory names) rather than overwriting a single one. This 
> make it easier for those coming to the review thread later to see 
> the evolution of the changes over time. 

> 
> +1
> 
> I had requested new test in the webrev during my review. That really
> helps me, a reviewer, to keep track what has been changed easily.  
> It will also give you an idea how many revisions this review 
> involves when you started for a code review (as opposed to asking 
> for "how to fix this issue").

Ah, that makes sense. Thanks for explaining. :)

> 
> I was asked to read the regression test that is attached to JBS issue 
[1]
> I was asked to review a diff (cut-n-paste) on the mail when I 
> requested a webrev to include a regression test. [2]
> 
> On Jan 31, 2019 [3], I includeed a link to the OpenJDK developer 
> guide and I was hoping you read the guideline and be familiar with 
> it which should help you contributing to the OpenJDK.

I completely forgot about that. I appreciate the reminder, will review the 
guide now so I don't forget again.

> 
> I was disappointed to get your conclusion:
> Historically, the bigger the change I propose, the more months it takes 
> the OpenJDK community to approve.

Perhaps I misspoke.

The only changes I've been able to get into OpenJDK have taken months 
apiece,
with the exception of typos and pure comment tweaks.

My *hope* is that small change sets will reduce the amount of debate (in 
general,
this is not aimed at you personally), and result in a faster turnaround.

> 
> I had helped as much as I can to sponsor this patch even if you 
> refused what I requested to help my review easier.

Refused? I don't recall refusing your help. I appreciate your help, and
I believe I have done everything you have asked, because I can see that
you are working hard to help me get this changeset in.

The only times I have not done as asked is when I have questions first,
to help me understand, or because I have legitimately forgotten.

> 
> I expected that you ran JDK regression tests on local machine rather
> than I caught it for you.  Is that what you expected a reviewer to 
> do it for you?   Won't you consider this a new revision to your 
> patch?  You can do anything you like and tells a reviewer that I 
> should be smart enough to identify the diff in the previous patch.  
> I am not an educator and excuse me if my response is not what you 
> are looking for.
> 
> Mandy

Do I expect reviewers to run tests for me? No. I made a mistake
by assuming this code change was so small that a test run beyond 
MethodHandlesGeneralTest was unnecessary.

This was a learning experience, and you did the right thing by
double-checking, and finding my mistake. Now I know better.

As for a new revision to my patch, I think you are referring to
the versioning mentioned above. Now you have kindly explained the
value of this to me, I am happy to include the versioning for 
future changes. Past changes may be tricky, as I don't have them.

Illustrates the benefits of versioning quite well, hehe.

As for your "educator" comment, you don't need to worry. Your
response was quite clear.

Best Regards

Adam Farley

> 
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-
> January/058320.html
> [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-
> February/058544.html
> [3] http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-
> January/058350.html
> [4] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-
> March/058784.html
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