On Fri, 3 Nov 2023 09:48:15 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Jan Lahoda has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Some more get->orElseThrow
>> - Reflecting review feedback.
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 338:
>
>> 336:
>> 337: @Override
>> 338: public Object apply(Integer labelIndex, Object value) {
>
> Since we already know the EnumDesc (i.e. `result`), we can just convert this
> to a `BiPredicate<Integer, Object>` for simplicity.
Done.
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 346:
>
>> 344: Class<?> clazz =
>> label.constantType().resolveConstantDesc(lookup);
>> 345:
>> 346: if (value.getClass() != clazz) {
>
> Not related to this patch, but this appears to be wrong: we should do
> something like
>
> if (!(value instanceof Enum<?> ev) || ev.getDeclaringClass() != clazz)
> return SENTINEL; // or false
You are right, of course, but let's solve that under
[JDK-8318144](https://bugs.openjdk.org/browse/JDK-8318144), to ensure that can
more easily be backported. Tests are running on the fix, will publish PR once
they pass. Thanks!
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 383:
>
>> 381: .withMethod("typeSwitch",
>> MethodTypeDesc.ofDescriptor("(Ljava/lang/Object;ILjava/util/function/BiFunction;)I"),
>> Classfile.ACC_STATIC, mb -> {
>> 382: mb.withFlags(AccessFlag.PUBLIC, AccessFlag.FINAL,
>> AccessFlag.STATIC)
>> 383: .withCode(cb -> {
>
> Can just use `clb.withMethodBody` and pass the access flags with
> `Classfile.ACC_PUBLIC | Classfile.ACC_STATIC | Classfile.ACC_FINAL`
Done.
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 432:
>
>> 430: cb.aload(2);
>> 431: cb.constantInstruction(enumIdx);
>> 432:
>> cb.invokestatic(Integer.class.describeConstable().get(),
>
> You can use `ConstantDescs.CD_Integer` etc.
Done.
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 434:
>
>> 432:
>> cb.invokestatic(Integer.class.describeConstable().get(),
>> 433: "valueOf",
>> 434:
>> MethodType.methodType(Integer.class,
>
> I recommend `MethodTypeDesc.of()` instead.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381868349
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381871678
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381866683
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381866943
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381867739
