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

[Jorn Vernee]

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. 
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/

Jorn

> Maurizio