On 8/23/21 4:07 PM, Brian Goetz wrote:

Actually, it will not NPE if `names` is null and you have selected equals/hashCode as the name.  Might be better to do requiresNonNull() up front for all the arguments, just to make such analysis simpler:

requireNonNull(methodName); requireNonNull(type); requireNonNull(recordClass); requireNonNull(names); requireNonNull(getters);

will do, thanks, Vicente

On 8/23/2021 4:04 PM, Brian Goetz wrote:

...

+1

On 8/23/2021 2:22 PM, 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)

-------------

Commit messages:   - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null

Changes: https://git.openjdk.java.net/jdk/pull/5226/files   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5226&range=00    Issue: https://bugs.openjdk.java.net/browse/JDK-8272347    Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 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

On Mon, 23 Aug 2021 23:13:58 GMT, Mandy Chung <mchung@openjdk.org> 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

On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung <mchung@openjdk.org> 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

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<mchung@openjdk.org> 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

On Tue, 24 Aug 2021 03:03:48 GMT, Vicente Romero <vromero@openjdk.org> 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

On Thu, 26 Aug 2021 02:36:47 GMT, Vicente Romero <vromero@openjdk.org> 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:

clarifying that the names parameter is ignored in some cases

Marked as reviewed by mchung (Reviewer). src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 345:

343: Arrays.stream(getters).forEach(Objects::requireNonNull); 344: MethodType methodType; 345: if (type instanceof MethodType)

Since you are modifying this file, do you mind taking Jesper's suggestion [1] posted in another PR to use pattern matching. Suggestion: if (type instanceof MethodType mt) methodType = mt; [1] https://github.com/openjdk/valhalla/pull/528#discussion_r688100918 ------------- PR: https://git.openjdk.java.net/jdk/pull/5226