Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On 8/25/21 4:45 PM, Mandy Chung wrote: On 8/25/21 12:08 PM, Vicente Romero wrote: On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung wrote: Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments. I think you meant requiring `names` to be non-null but `lookup` may still be null. It's okay to change the spec to require `names` to be non-null. Probably better to mention in `@param names` that `names` is ignored when the `methodName` is `equals` or `hashCode`. no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null. What I meant is to require it to be non-null but the spec should also mention `names` parameter is ignored if the method name is `equals` and `hashCode`. Mandy I see, I have updated the PR, thanks for your comments - PR:https://git.openjdk.java.net/jdk/pull/5226 Vicente
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On 8/25/21 12:08 PM, Vicente Romero wrote: On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung wrote: Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments. I think you meant requiring `names` to be non-null but `lookup` may still be null. It's okay to change the spec to require `names` to be non-null. Probably better to mention in `@param names` that `names` is ignored when the `methodName` is `equals` or `hashCode`. no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null. What I meant is to require it to be non-null but the spec should also mention `names` parameter is ignored if the method name is `equals` and `hashCode`. Mandy - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung wrote: >> Hi Mandy, I have changed the implementation of the method to explicitly >> require all arguments but lookup to be non-null as suggested by Brian. I >> have also covered, I think, all the missing test cases in test >> `ObjectMethodsTest`, thanks for your comments. > > I think you meant requiring `names` to be non-null but `lookup` may still be > null. It's okay to change the spec to require `names` to be non-null. > > Probably better to mention in `@param names` that `names` is ignored when the > `methodName` is `equals` or `hashCode`. no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null. - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On Tue, 24 Aug 2021 03:03:37 GMT, Vicente Romero wrote:
>> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 327:
>>
>>> 325: * @throws IllegalArgumentException if the bootstrap arguments are
>>> invalid
>>> 326: * or inconsistent
>>> 327: * @throws NullPointerException if any argument but {@code lookup}
>>> is {@code null}
>>
>> `names` may be null if the {@code methodName} is {@code "equals"} or {@code
>> "hashCode"}.This should be captured here.
>
> Hi Mandy, I have changed the implementation of the method to explicitly
> require all arguments but lookup to be non-null as suggested by Brian. I have
> also covered, I think, all the missing test cases in test
> `ObjectMethodsTest`, thanks for your comments.
I think you meant requiring `names` to be non-null but `lookup` may still be
null. It's okay to change the spec to require `names` to be non-null.
Probably better to mention in `@param names` that `names` is ignored when the
`methodName` is `equals` or `hashCode`.
-
PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On Tue, 24 Aug 2021 03:03:48 GMT, Vicente Romero wrote: >> Please review this simple PR along with the associated CSR. The PR is >> basically adding a line the the specification of method >> `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a >> NPE will be thrown. >> >> TIA >> >> link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > addressing review comments Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On Mon, 23 Aug 2021 23:13:58 GMT, Mandy Chung wrote:
>> Vicente Romero has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressing review comments
>
> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 327:
>
>> 325: * @throws IllegalArgumentException if the bootstrap arguments are
>> invalid
>> 326: * or inconsistent
>> 327: * @throws NullPointerException if any argument but {@code lookup}
>> is {@code null}
>
> `names` may be null if the {@code methodName} is {@code "equals"} or {@code
> "hashCode"}.This should be captured here.
Hi Mandy, I have changed the implementation of the method to explicitly require
all arguments but lookup to be non-null as suggested by Brian. I have also
covered, I think, all the missing test cases in test `ObjectMethodsTest`,
thanks for your comments.
-
PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
> Please review this simple PR along with the associated CSR. The PR is > basically adding a line the the specification of method > `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a > NPE will be thrown. > > TIA > > link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/b7489b41..89086ca1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5226=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5226=00-01 Stats: 30 lines in 2 files changed: 17 ins; 1 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
