On Mon, 4 Sep 2023 07:06:39 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:
>> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Remove unnecessary imports.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 695:
>
>> 693: * Negative [shiftAmount] shifts right and converts to int if needed.
>> 694: */
>> 695: record ShiftLeft(int shiftAmount, Class<?> type) implements Binding {
>
> Please split this into 2 binding operators: one for ShiftLeft and another for (arithmetic) ShiftRight, rather than mixing the 2 implementations into a single operator.
>
> For the right shift you could then also use a positive shift amount, and avoid the double negation that's needed right now.
Done.
> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 703:
>
>> 701: if (last != ((type == long.class) ? long.class : int.class))
>> 702: throw new IllegalArgumentException(
>> 703: String.format("Invalid operand type: %s. integral type expected", last));
>
> Why not use `SharedUtils.checkType` here?
>
> Suggestion:
>
> SharedUtils.checkType(last, type);
Done.
> src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 711:
>
>> 709: stack.push(type == long.class ? long.class : int.class);
>> 710: } else
>> 711: throw new IllegalArgumentException("shiftAmount 0 not supported");
>
> Please handle this validation in the static `shiftLeft` method. That's also where we do validation for other binding ops
Done.
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 737:
>
>> 735: cb.i2l();
>> 736: typeStack.pop();
>> 737: typeStack.push(long.class);
>
> Please use `popType` and `pushType` instead of manipulating the type stack manually. The former also does some verification. This should happen along every control path. Even if the binding is 1-to-1 (like this one) and doesn't change the type of the value, this would validate that the input type is correct, and make sure that any bugs are caught early.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314872201
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314871165
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314871596
PR Review Comment: https://git.openjdk.org/jdk/pull/15417#discussion_r1314871489