RFR: 8304283: Modernize the switch statements in jdk.internal.foreign [v2]

[Maurizio Cimadamore]

On Wed, 15 Mar 2023 19:38:53 GMT, Jorn Vernee <jvernee at 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