On Mon, 4 Sep 2023 07:06:39 GMT, Jorn Vernee <[email protected]> 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
