Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.htm... 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
On 7 Nov 2017, at 12:06, mandy chung <mandy.chung@oracle.com> wrote:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.htm...
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.
On 11/8/17 8:49 AM, Paul Sandoz wrote:
On 7 Nov 2017, at 12:06, mandy chung <mandy.chung@oracle.com> wrote:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.htm...
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
On 8 Nov 2017, at 10:13, mandy chung <mandy.chung@oracle.com> wrote:
On 11/8/17 8:49 AM, Paul Sandoz wrote:
On 7 Nov 2017, at 12:06, mandy chung <mandy.chung@oracle.com> <mailto:mandy.chung@oracle.com> wrote:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.htm... <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.
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 <mandy.chung@oracle.com> wrote:
On 11/8/17 8:49 AM, Paul Sandoz wrote:
On 7 Nov 2017, at 12:06, mandy chung <mandy.chung@oracle.com> <mailto:mandy.chung@oracle.com> wrote:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.htm... <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.
Hi Roger, Updated version: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.02/index.htm... 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
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.htm...
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
participants (3)
-
mandy chung
-
Paul Sandoz
-
Roger Riggs