Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

Mandy Chung
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html

This fixes the spec of MethodHandles::arrayLength, arrayConstructor,
arrayElementGetter/Setter to specify the behavior if the returned method
handle is invoked with null array or invalid index; same runtime
exception thrown by the bytecode behavior.  MethodHandle::asSpreader
spec is also clarified to throw NPE and IAE except when it spreads with
no argument and the returned method handle accepts a zero-length array
or null array.

Thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

Re: Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

Paul Sandoz

> On 7 Nov 2017, at 12:06, mandy chung <[hidden email]> wrote:
>
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html
>
> This fixes the spec of MethodHandles::arrayLength, arrayConstructor, arrayElementGetter/Setter to specify the behavior if the returned method handle is invoked with null array or invalid index; same runtime exception thrown by the bytecode behavior.  MethodHandle::asSpreader spec is also clarified to throw NPE and IAE except when it spreads with no argument and the returned method handle accepts a zero-length array or null array.
>

Looks good, minor comment.

MethodHandle
--

 889      * When the adapter is called, the length of the supplied {@code array}
 890      * argument is queried as if by {@code array.length} or {@code arrayLength}

s/arrayLength/arraylength/

See also MethodHandles.

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

Mandy Chung


On 11/8/17 8:49 AM, Paul Sandoz wrote:

>> On 7 Nov 2017, at 12:06, mandy chung <[hidden email]> wrote:
>>
>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html
>>
>> This fixes the spec of MethodHandles::arrayLength, arrayConstructor, arrayElementGetter/Setter to specify the behavior if the returned method handle is invoked with null array or invalid index; same runtime exception thrown by the bytecode behavior.  MethodHandle::asSpreader spec is also clarified to throw NPE and IAE except when it spreads with no argument and the returned method handle accepts a zero-length array or null array.
>>
> Looks good, minor comment.
>
> MethodHandle
> --
>
>   889      * When the adapter is called, the length of the supplied {@code array}
>   890      * argument is queried as if by {@code array.length} or {@code arrayLength}
>
> s/arrayLength/arraylength/
>
> See also MethodHandles.
>

Oops typo.  Fixed.

http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.01/

Here is the CSR:
    https://bugs.openjdk.java.net/browse/JDK-8190945

Can you also review it?

thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

Re: Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

Paul Sandoz

> On 8 Nov 2017, at 10:13, mandy chung <[hidden email]> wrote:
>
>
>
> On 11/8/17 8:49 AM, Paul Sandoz wrote:
>>> On 7 Nov 2017, at 12:06, mandy chung <[hidden email]> <mailto:[hidden email]> wrote:
>>>
>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html <http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html>
>>>
>>> This fixes the spec of MethodHandles::arrayLength, arrayConstructor, arrayElementGetter/Setter to specify the behavior if the returned method handle is invoked with null array or invalid index; same runtime exception thrown by the bytecode behavior.  MethodHandle::asSpreader spec is also clarified to throw NPE and IAE except when it spreads with no argument and the returned method handle accepts a zero-length array or null array.
>>>
>> Looks good, minor comment.
>>
>> MethodHandle
>> --
>>
>>  889      * When the adapter is called, the length of the supplied {@code array}
>>  890      * argument is queried as if by {@code array.length} or {@code arrayLength}
>>
>> s/arrayLength/arraylength/
>>
>> See also MethodHandles.
>>
>
> Oops typo.  Fixed.
>
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.01/ <http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.01/>
>
> Here is the CSR:
>    https://bugs.openjdk.java.net/browse/JDK-8190945 <https://bugs.openjdk.java.net/browse/JDK-8190945>
>
> Can you also review it?
>

Reviewed.

Paul.
Reply | Threaded
Open this post in threaded view
|

Re: Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

roger riggs
Hi Mandy,

A few editorial suggestions:

MethodHandle.java:
891: "accepts**a** zero-length trailing array argument
895: "if **the* *{@code array} does"

MethodHandleImpl.java:
667:  hard to follow indentation; perhaps adding the "else" will help.

MethodHandles.java:
2517: "Produces a method handle *for* constructing arrays..."
2523:  "array size, *a* {@code NegativeArraySizeException} will be thrown."
2550: "array reference, *a* {@code NullPointerPointer} exception will be
thrown."
2573 & 2598 & 2641: "*A* {@code NullPointerPointer} *exception* will be
thrown if the array reference

Is there a test for asSpreader(null, 0) and asSpreader(null, 1)? (The
new exception thrown at MethodHandleImpl:667)

Roger


On 11/8/2017 1:20 PM, Paul Sandoz wrote:

>> On 8 Nov 2017, at 10:13, mandy chung <[hidden email]> wrote:
>>
>>
>>
>> On 11/8/17 8:49 AM, Paul Sandoz wrote:
>>>> On 7 Nov 2017, at 12:06, mandy chung <[hidden email]> <mailto:[hidden email]> wrote:
>>>>
>>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html <http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html>
>>>>
>>>> This fixes the spec of MethodHandles::arrayLength, arrayConstructor, arrayElementGetter/Setter to specify the behavior if the returned method handle is invoked with null array or invalid index; same runtime exception thrown by the bytecode behavior.  MethodHandle::asSpreader spec is also clarified to throw NPE and IAE except when it spreads with no argument and the returned method handle accepts a zero-length array or null array.
>>>>
>>> Looks good, minor comment.
>>>
>>> MethodHandle
>>> --
>>>
>>>   889      * When the adapter is called, the length of the supplied {@code array}
>>>   890      * argument is queried as if by {@code array.length} or {@code arrayLength}
>>>
>>> s/arrayLength/arraylength/
>>>
>>> See also MethodHandles.
>>>
>> Oops typo.  Fixed.
>>
>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.01/ <http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.01/>
>>
>> Here is the CSR:
>>     https://bugs.openjdk.java.net/browse/JDK-8190945 <https://bugs.openjdk.java.net/browse/JDK-8190945>
>>
>> Can you also review it?
>>
> Reviewed.
>
> Paul.

Reply | Threaded
Open this post in threaded view
|

Re: Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

Mandy Chung
Hi Roger,

Updated version:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.02/index.html

On 11/8/17 11:37 AM, Roger Riggs wrote:
> Hi Mandy,
>
> A few editorial suggestions:
>
> MethodHandle.java:
> 891: "accepts**a** zero-length trailing array argument

Fixed
> 895: "if **the* *{@code array} does"
>

{@code array} is referred to the parameter name where I don't use
article.  I drop the {@code...} and change it to "If the array".

> MethodHandleImpl.java:
> 667:  hard to follow indentation; perhaps adding the "else" will help.
>

I revised it to the following:

         if (av == null && n == 0) {
             return;
         } else if (av == null) {
             throw new NullPointerException("null array reference");

> MethodHandles.java:
> 2517: "Produces a method handle *for* constructing arrays..."

It reads fine without "for" and the other arrayXXX factory methods are
similar.  I leave it for the original author to follow up.

> 2523: "array size, *a* {@code NegativeArraySizeException} will be
> thrown."
> 2550: "array reference, *a* {@code NullPointerPointer} exception will
> be thrown."
> 2573 & 2598 & 2641: "*A* {@code NullPointerPointer} *exception* will
> be thrown if the array reference
>
> Is there a test for asSpreader(null, 0) and asSpreader(null, 1)? (The
> new exception thrown at MethodHandleImpl:667)
>

The new exception is thrown when the MethodHandle returned by
MethodHandle::asSpreader.  The test cases are covered by the new
InvokeMethodHandleWithBadArgument test.

asSpreader(null, 0) and asSpreader(null, 1) are not related to this
issue.  The check is done by MethodHandle::asSpreaderChecks. Since this
gets my attention, I add the test cases in an existing test
SpreadCollectTest.java.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

roger riggs
Looks fine.

Nothing more from me,   Roger

On 11/8/2017 6:52 PM, mandy chung wrote:

> Hi Roger,
>
> Updated version:
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.02/index.html 
>
>
> On 11/8/17 11:37 AM, Roger Riggs wrote:
>> Hi Mandy,
>>
>> A few editorial suggestions:
>>
>> MethodHandle.java:
>> 891: "accepts**a** zero-length trailing array argument
>
> Fixed
>> 895: "if **the* *{@code array} does"
>>
>
> {@code array} is referred to the parameter name where I don't use
> article.  I drop the {@code...} and change it to "If the array".
>
>> MethodHandleImpl.java:
>> 667:  hard to follow indentation; perhaps adding the "else" will help.
>>
>
> I revised it to the following:
>
>         if (av == null && n == 0) {
>             return;
>         } else if (av == null) {
>             throw new NullPointerException("null array reference");
>
>> MethodHandles.java:
>> 2517: "Produces a method handle *for* constructing arrays..."
>
> It reads fine without "for" and the other arrayXXX factory methods are
> similar.  I leave it for the original author to follow up.
>
>> 2523: "array size, *a* {@code NegativeArraySizeException} will be
>> thrown."
>> 2550: "array reference, *a* {@code NullPointerPointer} exception will
>> be thrown."
>> 2573 & 2598 & 2641: "*A* {@code NullPointerPointer} *exception* will
>> be thrown if the array reference
>>
>> Is there a test for asSpreader(null, 0) and asSpreader(null, 1)? (The
>> new exception thrown at MethodHandleImpl:667)
>>
>
> The new exception is thrown when the MethodHandle returned by
> MethodHandle::asSpreader.  The test cases are covered by the new
> InvokeMethodHandleWithBadArgument test.
>
> asSpreader(null, 0) and asSpreader(null, 1) are not related to this
> issue.  The check is done by MethodHandle::asSpreaderChecks. Since
> this gets my attention, I add the test cases in an existing test
> SpreadCollectTest.java.
>
> Mandy