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&pr=5226&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=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
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]
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 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 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 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 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