RFR 8187742 Minimal set of bootstrap methods for dynamic constants

[Paul Sandoz]

Hi Remi,

You are asking a lot of the same questions we went through a number of times before landing where we are :-)


> On 7 Nov 2017, at 11:33, Remi Forax <forax at univ-mlv.fr> wrote:
> 
> Hi Paul,
> 
> You have an import static requireNonNull but it is only used once in nullConstant, all other methods use Objects.requireNonNull.
> 

Fixed.


> The test that checks that lookup, name and type are null or not is different in each method,
> so by example in primitiveClass, if type equals null, you get an IAE. 
> 

Fixed.


> I fail to see the point of using validateClassAccess(), if the BSM is used through an indy, the type is created only if the caller class can create it, so the test seems redundant. If it's not used by an indy, why do we need to test that ?

We are being deliberately conservative ensuring the lookup is consistent with the type in case nefarious byte code spinners punch a hole (not proven a hole can be punched, just being conservative).


> Also, why it's not called in invoke ?
> 

What “it” are you referring to?


> There is also no reason to validate that the parameter type is the right one because the VM will validate the return type of the BSM is asTypable to the type sent as argument.
> 

Yes, but we prefer that the BSM reject thereby better signalling the source of the error.


> Some methods use generics, so other don't. Given it's not useful to use generics if the methods is used as a BSM, i think the generics signature should be removed, otherwise, getstatic and invoke should use generics.
> 

I updated the nullConstant method and removed the type variable.

In general we try to be consistent with the explicit erasure unless it causes some contortion in the implementation as is the case for Enum.


> In nullConstant, you do not need a requireNonNull here, getting you call isPrimitive() just after.
> 

We decided to be explicit rather than implicit for all null checks.


> In primitiveClass, the second test is equivalent to name.length() != 1, to remove the @SuppressWarnings, type should be declared as a Class<Class<?>>. Why not using getstatic to retrieve the field Type of the wrapper instead ?
> 

Try passing Class.class to it. To be honest this is somewhat motivated by testing when called explicitly.


> If you have invoke(), you do not need enumConstant because you can cook a constant method handle Enum.valueOf and send it to invoke.

It’s also possible to support via getStatic as well, as is the case for primitive class. 

We went back and forth on the generic and specific axis. For cases where we considered constants are “honorary" members of the constant pool we provide explicit BSMs.


> The methods that returns VarHandles are trickier because you need a Lookup object. 
> 
> getstatic should be renamed to getConstant because this is what it does.

No, we want to stick closely with the notion of what the BSM does, there is a close connection also with MethodHandle getStatic, it’s performing a static field access. “getConstant” is too vague, notionally all the BSMs return constants.


> wrapping the Throwable in a LinkageError seems wrong to me given that a calls to a BSM already does that, so getstatic can just declare "throws Throawble" like invoke and you have the same semantics.
> 

We don’t want to declare Throwable for the API in this case. This try/catch is just ceremony since a getstatic MH in the implementation can only throw a VM error e.g. related to out of memory or stack overflow. I can add some comments in the code.

Note that reflective exceptions are mapped to their equivalent errors. Although the BSM has been linked it is being asked to perform what can be considered further linkage.


> in arrayVarHandle, the doc said that lookup is not used but lookup is used.
> 

Fixed.


> in validateClassAccess, the return can be moved at the end of the method.
> 
> and as a minor issue, all the ifs should be followed by curly braces per the coding convention.
> 

I expanded to curly braces.

Paul.