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

[Jorn Vernee]

Ok, I'll go with v01 then.

Thanks,
Jorn

On 23/01/2020 18:35, Maurizio Cimadamore wrote:
> I don't have strong feelings here - and I concur that efficiency of 
> this code is probably a second-order issue (at least at present).
>
> That said, I don't find your first incremental webrev to be so 
> unreadable, and it's equally maintainable, in the sense that if you 
> add a new field, javac will tell you that you forgot to initialize it.
>
> So, to me the choice is between the original webrev and v01. v01b is 
> clever, but feels like overkill to me.
>
> Maurizio
>
>
> On 23/01/2020 17:06, Jorn Vernee wrote:
>> 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