[foreign-memaccess] RFR 8225172: Add way to create deref VarHandles without using the Layout API

[Maurizio Cimadamore]

On 13/06/2019 17:53, Jorn Vernee wrote:
> On 2019-06-13 17:56, Maurizio Cimadamore wrote:
>> On 13/06/2019 16:40, Jorn Vernee wrote:
>>> Updated webrev: 
>>> https://cr.openjdk.java.net/~jvernee/panama/webrevs/8225172/webrev.04/
>>>
>>> The sizeOf(Class) call was also implicitly doing a carrier type 
>>> check, so I'm now doing that explicitly in 
>>> MemoryAccessVarHandles::dereferenceVarHandle and 
>>> LayoutPathImpl::dereferenceVarHandle instead, and added a test case 
>>> for this as well.
>>
>> Looks nice...
>>
>> Of these two checks:
>>
>> if (!carrier.isPrimitive() || carrier == void.class || carrier ==
>> boolean.class) {
>> +            throw new IllegalArgumentException("Illegal carrier: " +
>> carrier.getSimpleName());
>> +        }
>> +
>> +        if (Wrapper.forPrimitiveType(carrier).bitWidth() !=
>> layout.bitsSize()) {
>> +            throw new IllegalArgumentException("Invalid carrier: " +
>> carrier + ", for layout " + layout);
>> +        }
>>
>>
>> The former seems redundant. BitWidth of void is zero, and the
>> ValueLayout API throws IAE if size is 0.
>>
>> So I would rewrite as this:
>>
>> Wrapper wrapper = Wrapper.forPrimitiveType(carrier);
>> if (wrapper == null || //not a primitive
>>     wrapper.bitWidth() != layout.bitSize()) { //incompatible sizes (or
>> void carrier!)
>>   throw new IllegalArgumentException(...)
>> }
>
> Note that forPrimitiveType also throws an exception if a non-primitive 
> class is passed, and we also need to check for boolean, which is 
> illegal in our case. So, I deemed it cleaner to do an explicit check, 
> and get the same exception message in both (non-primitve and boolean) 
> cases. 
Whoops - I was looking at 'Wrapper::findPrimitiveType' but that's not 
accessible from outside the sun.invoke.util package. Also true that we 
need to guard against boolean.
> But, the if statements can be merged there. I've left in the check for 
> void.class to make it clearer from the code that that's also an 
> illegal carrier.
>
>> And, again, if you check the carrier both in MemoryAccessVarHandles,
>> and in LayoutPath, I don't see the need for checking it in VarHandles.
>
> Yeah OK, this was kind of a safety feature in case 
> makeMemoryAccessVarHandle were to be called from a different place, 
> but it's not really needed with the current implementation. I've 
> removed redundant checks and moved the alignment check to 
> MemoryAccessVarHandle as well. Layout does similar checking of the 
> alignment already.
>
> Updated webrev: 
> https://cr.openjdk.java.net/~jvernee/panama/webrevs/8225172/webrev.05/

This looks good - I'd also be ok with moving this:

if (!carrier.isPrimitive() || carrier == void.class || carrier == 
boolean.class) {
   60             throw new IllegalArgumentException("Illegal carrier: " 
+ carrier.getSimpleName());
   61         }

To VarHandleMemoryAddressBase (as a static method), which you can then 
reuse in both places. If you opt for this, no need for another RFR.

Maurizio


>
> Jorn
>
>> Maurizio