[jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE
Mandy Chung
mchung at openjdk.java.net
Mon Jul 12 17:18:51 UTC 2021
On Sat, 10 Jul 2021 19:03:36 GMT, Vicente Romero <vromero at openjdk.org> wrote:
> Please review this PR that is fixing a mismatch between the implementation for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its implementation. I made a mistake while working on a recent CSR [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the API but mistakenly thought that the implementation was in sync with the spec. This is why this change is also including a unit test of the API for `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` which is covered in test `IndyDescTest` in the same test suite. Also this change needs a CSR as while fixing the implementation of method `::withArgs` I realized that the API of the varargs overloaded version of method `::of` needed some rewording too as it is invoking the same private constructor `::withArgs` is invoking. So it didn't make sense for the API of one method to be more restrictive than the other. Please review also the accompanying CSR.
>
> Thanks,
> Vicente
Looks good. Minor suggestions in the test.
test/jdk/java/lang/constant/DynamicCallSiteDescTest.java line 60:
> 58: "bootstrap",
> 59: ClassDesc.of("java.lang.invoke.CallSite")
> 60: ),
A minor suggestion: you can have the return `DirectMethodHandleDesc` in a local variable to be used in all `DynamicCallSiteDesc::of` calls that would avoid copy-n-paste of same `ConstantDescs::ofCallsiteBootstrap` call.
test/jdk/java/lang/constant/DynamicCallSiteDescTest.java line 64:
> 62: MethodTypeDesc.ofDescriptor("()I")
> 63: );
> 64: throw new AssertionError("IllegalArgumentException expected");
Suggestion:
fail("IllegalArgumentException expected");
Same for other `throw new AssertionError(...)`
-------------
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk17/pull/242
More information about the core-libs-dev
mailing list