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

[Maurizio Cimadamore]

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