[foreign-abi] RFR 8237762: Add platform dependent MemoryLayout constants

[Jorn Vernee]

Hi Rémi,

Thanks for the suggestion. As Maurizio said, we don't need the local 
variables and can assign to the fields directly.

Incremental webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8237762/webrev.01-inc/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayouts.java.udiff.html

A solution that scales better, and also avoid comparing the ABI many 
times would be to create the picking behaviour dynamically.

Incremental webrev: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8237762/webrev.01b-inc/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayouts.java.udiff.html

Though, I think that will be even slower, since I'm assuming this code 
will only ever run a handful of times in the interpreter.

Since it will only run a few times, I'm not so worried about the 
efficiency of this code. I'd say let's optimize for 
readability/simplicity for now. What do you think?

Jorn

On 23/01/2020 17:20, Remi Forax wrote:
> Hi Jorn,
> that seems very inefficient, because you are testing the ABI for each static final,
> you can do it once in the static block.
>
> static {
>    var c_bool;
>    switch (abi.name()) {
>      case ABI_SYSV -> {
>        c_bool = ...
>      }
>      case ABI_WINDOWS ->  {
>        c_bool = ...
>      }
>      case ABI_AARCH64 -> {
>        c_bool = ...
>      }
>      default -> throw new UnsupportedOperationException("Unsupported ABI: " + abi.name());
>    }
>    C_BOOL = c_bool;
> }
>
> This solution will not scale well because you need one local variable per type, but that should be good enough in this case.
>
> regards,
> Rémi
>
> ----- Mail original -----
>> De: "Jorn Vernee" <jorn.vernee at oracle.com>
>> À: "panama-dev at openjdk.java.net'" <panama-dev at openjdk.java.net>
>> Envoyé: Jeudi 23 Janvier 2020 17:03:02
>> Objet: [foreign-abi] RFR 8237762: Add platform dependent MemoryLayout constants
>> Hi,
>>
>> Please review the following patch that adds platform/ABI dependent
>> MemoryLayout constants to MemoryLayouts.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8237762
>> Webrev:
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8237762/webrev.00/
>>
>> Thanks,
>> Jorn