On Thu, 16 Mar 2023 09:40:30 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> 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.
I've spoken to the Amber persons and soon we will be able to do:
static boolean isBox(Binding binding) {
return switch (binding) {
case VMLoad _,
Copy _,
Allocate _,
BoxAddress _,
BufferStore _,
Dup _,
Cast _-> true;
case VMStore _,
BufferLoad _,
UnboxAddress _ -> false;
};
}
-------------
PR: https://git.openjdk.org/jdk/pull/13047
