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

[Per Minborg]

On Thu, 16 Mar 2023 09:40:30 GMT, Maurizio Cimadamore <mcimadamore at 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