On Wed, 15 Mar 2023 19:38:53 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reintroduce missing comment
>
> src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java
> line 219:
>
>> 217: case Binding.Cast unused-> true;
>> 218: };
>> 219: }
>
> I'd go a bit further here and visually organize the cases as well. Also,
> using a static import for `Binding.*`:
>
> Suggestion:
>
> static boolean isUnbox(Binding binding) {
> return switch (binding) {
> case VMStore unused -> true;
> case BufferLoad unused -> true;
> case Copy unused -> true;
> case UnboxAddress unused -> true;
> case Dup unused -> true;
> case Cast unused -> true;
>
> case VMLoad unused -> false;
> case BufferStore unused -> false;
> case Allocate unused -> false;
> case BoxAddress unused -> false;
> };
> }
>
>
> It's a bit unfortunate that we need variable names as well.
While I agree that some "visitor" methods would be better dealt with using
patterns, I remain unconvinced about the box/unbox classification being modeled
as an "operation". That's because it's a static property of the binding - e.g.
given a binding you know whether it can be used for box/unbox/both/none. If
this was an enum, I would encode it as a boolean field and never look back.
Also note how the javadoc for Binding itself makes quite a lot of comments on
box/unbox nature of bindings, and how they can have different semantics
depending on the direction. To me it feels like that distinction is a first
class citizen in Binding.
Perhaps, pulling together the various strings, it would maybe possible to
define very simple records for the various bindings, with no verification and
interpretation code (e.g. leave that to some pattern-based visitor somewhere
else). But I think I would still expect a binding class to know whether it's
used for unbox or not w/o having to run a complex operation all the time (given
that the property is a constant of the binding class). The fact that we're
using records for bindings and we can't have an abstract class also biases the
decision a bit (otherwise we could simply use a bunch of constructor parameters
to encode the box/unbox properties).
That said, this is all subjective. I don't have super strong opinion on this.
The code above looks a tad odd to me, but I can live with it.
-------------
PR: https://git.openjdk.org/jdk/pull/13047
