On Fri, 26 Apr 2024 18:41:18 GMT, Claes Redestad <[email protected]> wrote:
>> src/java.base/share/classes/sun/invoke/util/Wrapper.java line 384:
>>
>>> 382:
>>> 383: /** A nominal descriptor of the primitive type */
>>> 384: public ClassDesc primitiveClassDescriptor() { return
>>> primitiveTypeDesc; }
>>
>> As this method is named as `primitiveClassDescriptor`, I expect this should
>> either throw or return null for `OBJECT` and even `VOID` wrapper. Or
>> should be named "classDescriptor".
>
> While I agree it may seem non-sensical to define `CD_Object` for `Object`,
> there's precedent in that `Wrapper.primitiveType` is `Object.class` - so for
> consistency I wired it up the same way. For the usage patterns introduced
> with this patch we'll never call `primitiveClassDescriptor()` on
> `Wrapper.OBJECT`, though - and `Wrapper.forPrimitiveType` will throw an `IAE`
> if you try - so we can always revisit this.
>
> `void.class` is the primitive type for `Void.class`, allowed as a return type
> and recognized by `Wrapper.forPrimitiveType`. There are checks in for example
> `ConstantUtils.parseMethodDescriptor` which disallows `void.class` anywhere
> but as a return type.
I know it'll never call `primitiveClassDescriptor()` besides primitive types.
While `void.class.isPrimitive()` returns true, `void` is not a primitive type
and called out explicitly in the spec.
My point is about the behavior does not match the method name. It may be
better to rename it to `classDescriptor` instead. It's okay to clean up in
your other performance fix (perhaps #18945).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18971#discussion_r1581507425
