Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

2018-05-22 Thread mandy chung 

+1

Mandy

On 5/22/18 6:00 PM, Vivek Theeyarath wrote:


Thanks for the comments Mandy. I have updated the test accordingly.

http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.04/ 
<http://cr.openjdk.java.net/%7Evtheeyarath/8177276/webrev.04/>


Regards

Vivek

*From:*mandy chung
*Sent:* Tuesday, May 22, 2018 10:07 PM
*To:* Vivek Theeyarath <vivek.theeyar...@oracle.com>; Paul Sandoz 
<paul.san...@oracle.com>
*Cc:* core-libs-dev <core-libs-dev@openjdk.java.net>; Nadeesh TV 
<nadeesh.thatathil.vala...@mdcpartners.be>
*Subject:* Re: RFR: 8177276: MethodHandles.insertArguments doesn't 
specify IllegalArgumentException on index mismatch


On 5/22/18 9:09 AM, Vivek Theeyarath wrote:

Hi All,

   Thanks for the comments. I have incorporated the changes as per 
Nadeesh's and Paul's comments. The test runs fine with jtreg post the changes. 
Also, the typo errors Mandy pointed out has also been fixed. Please find the 
updated webrev.

http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.03/
<http://cr.openjdk.java.net/%7Evtheeyarath/8177276/webrev.03/>  



Looks fine.   Nit:  line 119-133 in 
MethodHandlesInsertArgumentsTest.java can be converted into two  
@Test(expected = ClassCastException.class) cases like the other two 
IAE cases.  You can update it before you push.


Mandy

RE: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

2018-05-22 Thread Vivek Theeyarath 
Thanks for the comments Mandy. I have updated the test accordingly.

 

http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.04/ 

 

Regards

Vivek

From: mandy chung 
Sent: Tuesday, May 22, 2018 10:07 PM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>; Paul Sandoz 
<paul.san...@oracle.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; Nadeesh TV 
<nadeesh.thatathil.vala...@mdcpartners.be>
Subject: Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify 
IllegalArgumentException on index mismatch

 

 

On 5/22/18 9:09 AM, Vivek Theeyarath wrote:

Hi All,
  Thanks for the comments. I have incorporated the changes as per Nadeesh's and 
Paul's comments. The test runs fine with jtreg post the changes. Also, the typo 
errors Mandy pointed out has also been fixed. Please find the updated webrev.
 
http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.03/ 


Looks fine.   Nit:  line 119-133 in MethodHandlesInsertArgumentsTest.java can 
be converted into two  @Test(expected = ClassCastException.class) cases like 
the other two IAE cases.  You can update it before you push.

Mandy

Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

2018-05-22 Thread mandy chung 



On 5/22/18 9:09 AM, Vivek Theeyarath wrote:

Hi All,
Thanks for the comments. I have incorporated the changes as per 
Nadeesh's and Paul's comments. The test runs fine with jtreg post the changes. 
Also, the typo errors Mandy pointed out has also been fixed. Please find the 
updated webrev.

http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.03/


Looks fine.   Nit:  line 119-133 in 
MethodHandlesInsertArgumentsTest.java can be converted into two  
@Test(expected = ClassCastException.class) cases like the other two IAE 
cases.  You can update it before you push.


Mandy

Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

2018-05-22 Thread Paul Sandoz 
+1

Paul.

> On May 22, 2018, at 9:09 AM, Vivek Theeyarath  
> wrote:
> 
> Hi All,
>   Thanks for the comments. I have incorporated the changes as per 
> Nadeesh's and Paul's comments. The test runs fine with jtreg post the 
> changes. Also, the typo errors Mandy pointed out has also been fixed. Please 
> find the updated webrev.
> 
> http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.03/ 
> 
> Bug : https://bugs.openjdk.java.net/browse/JDK-8177276 
> CSR : https://bugs.openjdk.java.net/browse/JDK-8202991 
> 
> Regards
> Vivek

RE: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

2018-05-22 Thread Vivek Theeyarath 
Hi All,
Thanks for the comments. I have incorporated the changes as per 
Nadeesh's and Paul's comments. The test runs fine with jtreg post the changes. 
Also, the typo errors Mandy pointed out has also been fixed. Please find the 
updated webrev.

http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.03/ 

Bug : https://bugs.openjdk.java.net/browse/JDK-8177276 
CSR : https://bugs.openjdk.java.net/browse/JDK-8202991 

Regards
Vivek
-Original Message-
From: Paul Sandoz 
Sent: Monday, May 21, 2018 10:19 PM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; Nadeesh TV 
<nadeesh.thatathil.vala...@mdcpartners.be>
Subject: Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify 
IllegalArgumentException on index mismatch



> On May 19, 2018, at 7:14 AM, Nadeesh TV 
> <nadeesh.thatathil.vala...@mdcpartners.be> wrote:
> 
> Hi Vivek,
> 
> IMHO, assigning back to methodHandle on  line 109, 115, 122,123 is confusing
> 

MethodHandlesInsertArgumentsTest.java
—

Yes, not just confusing but incorrect, the updated static field will affect 
what is tested so you are dependent on the order of test method execution. Make 
the field final and use the convention for static field names, and drop the 
assignment for the insertArgument calls.


 118 @Test
 119 public void testInsertArgumentsPosZero() {
 120 countTest();
 121 try {
 122 methodHandle = MethodHandles.insertArguments(methodHandle, 0, 
"First");
 123 methodHandle = MethodHandles.insertArguments(methodHandle, 1, 
"First", new Object());
 124 Assert.fail("ClassCastException not thrown");
 125 }
 126 catch(ClassCastException cce) {
 127 }
 128 }

You can split this out into multiple try/catch, one for each 
MethodHandles.insertArguments call so the assertion directly captures the 
failure point, otherwise declare the exception in the @Test.


MethodHandles.java
—

3489  * @throws ClassCastException If an argument does not mach the 
corresponding bound parameter

Lower case the “If”

Paul.

Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

2018-05-21 Thread mandy chung 



On 5/16/18 5:51 PM, Vivek Theeyarath wrote:

Hi All,

  Please review fix for 
https://bugs.openjdk.java.net/browse/JDK-8177276 .

http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.02/


3489  * @throws ClassCastException If an argument does not mach the 
corresponding bound parameter


a typo "mach" in addition to what Paul points out (lower case the "If")

I will review the test when you send a new webrev to address Paul's comment.

Mandy

Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch

2018-05-21 Thread Paul Sandoz 


> On May 19, 2018, at 7:14 AM, Nadeesh TV 
>  wrote:
> 
> Hi Vivek,
> 
> IMHO, assigning back to methodHandle on  line 109, 115, 122,123 is confusing
> 

MethodHandlesInsertArgumentsTest.java
—

Yes, not just confusing but incorrect, the updated static field will affect 
what is tested so you are dependent on the order of test method execution. Make 
the field final and use the convention for static field names, and drop the 
assignment for the insertArgument calls.


 118 @Test
 119 public void testInsertArgumentsPosZero() {
 120 countTest();
 121 try {
 122 methodHandle = MethodHandles.insertArguments(methodHandle, 0, 
"First");
 123 methodHandle = MethodHandles.insertArguments(methodHandle, 1, 
"First", new Object());
 124 Assert.fail("ClassCastException not thrown");
 125 }
 126 catch(ClassCastException cce) {
 127 }
 128 }

You can split this out into multiple try/catch, one for each 
MethodHandles.insertArguments call so the assertion directly captures the 
failure point, otherwise declare the exception in the @Test.


MethodHandles.java
—

3489  * @throws ClassCastException If an argument does not mach the 
corresponding bound parameter

Lower case the “If”

Paul.