RFR: implementation for JEP 334: JVM Constants API

jbvernee jbvernee at xs4all.nl
Fri Jun 1 13:04:32 UTC 2018


(Thanks, Brian, for your earlier response. I've been interested in the 
constants API/condy/constant propagation since first hearing about it 
last year, so it's great to be able to get involved :)

I have some more code review comments. This time I looked at the 
implementations as well. I hope it's okay for a non-member to give 
comments about that, but after writing them down I didn't want to just 
throw them away.

They're organized by file:


- For `arrayType()` you can change it to return `Class<T[]>` (useful for 
reflection), which is a valid sub-signature. (I mentioned this before, 
but it seems to have fallen through the cracks)


- For `EnumDesc.constantBootstrap` I think you can require a `ClassDesc` 
directly as a parameter instead of a descriptor string that you then 
have to convert to a ClassDesc? (When you use it in `describeConstable` 
you have to convert your ClassDesc to a descriptor string. It'd be 
simpler if you could just pass the ClassDesc)


- For `VarHandleDesc.Kind.toBSMArgs` you could change it to return a 
`ConstantDesc<?>[]`, since the only use case requires an array. (also, 
see the next comment)

- In `VarHandleDesc.describeConstable` you can use `Constable<?>[] args 
= kind.toBSMArgs(declaringClass, constantName(), varType)` (assuming the 
changed return type), instead of having the ternary.


- In the different `describeConstable` maybe use `orElseThrow()` instead 
of `get()` on the optionals. (I think get() will be deprecated soon?)

- For the last one (line 662), instead of doing an explicit `isPresent` 
check, you can also use `Optional::map`: `return 
arrayTypeRef.map(VarHandleDesc::ofArray);`. (If it's ok to use indy 


- Some of the string manipulation going on with descriptor strings looks 
kinda cryptic (for instance, several places use `descriptor.length() == 
1`, instead of a more descriptive function call, e.g. 
`isPrimitveDescriptor(descriptor)`). It might be a good idea to factor 
out the nitty gritty string manipulation into a DescriptorStrings class 
as static helpers, and use static imports (I don't know if that exists 
in the JDK, but there seems to be a wide demand for it).

- In `arrayType(int)`, add an exception message here. e.g.: `"rank must 
be positive, but was: " + rank`.


- In the constructor, I suggest factoring out the if-condition to an 
`isReferenceDescriptor(String descriptor)` helper. (more descriptive)

- In `resolveConstantDesc` (line 77): You can use `if(c.isPrimitive())` 
instead of `c.descriptorString().length() == 1`

- In the same method (line 82): You can use `clazz = clazz.arrayType()` 
instead of `clazz = Array.newInstance(clazz, 0).getClass();`


- The `resolveArgs` method uses an odd dance to propagate the exception 
thrown from the lambda, imho it's better to just use a traditional 
for-loop here instead of the stream.

Best regards,
Jorn Vernee

More information about the amber-dev mailing list