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

[Jorn Vernee]

Thanks for the review! Pushed.

I've left the check as is, since VarHandleMemoryAddressBase is package 
private in j.l.i. I'm not sure what a good place for a shared util would 
be, since it would need to be public. I think j.l.i is no good since 
it's an exported package, although it otherwise seems like the right 
place to put such a shared util (maybe we need jdk.internal.invoke?).

Jorn

On 2019-06-13 19:09, Maurizio Cimadamore wrote:
> 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