This is the terminology used by the JVMS (JVMS 4.3.2).  Since these things model classfile entities, we adopted the JVMS terminology. On 5/24/2018 1:32 AM, Ali Ebrahimi wrote:

I think using FieldTypeDescriptor is misleading. since that is used for param types and return types. I propose SimpleTypeDescriptor or VariableTypeDescriptor.

On Wed, May 23, 2018 at 11:11 PM, Vicente Romero <vicente.romero@oracle.com> wrote:

...

Hi all,

Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].

Thanks, Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/ constants.api.patch [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/ overview-summary.html

PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)

Thanks for the review and the great comments!

This version is far better than the previous one,

... or the previous seven :)

MethodHandleDesc.Kind: The JVMS defines names for these constants, why using different names here ?

Mostly for readability of client code.  Happy to consider alternatives.

ClassDesc.isClassOrInterface() seems very specific, does it includes annotation or enums, how will it works when value types will be included ?

Hah, this was added based on a previous review, where someone said "you have isArray() and isPrimitive(), but there are methods that only work on non-array, non-primitive (e.g., packageName()), so you should have a corresponding method for this, and then the isXxx methods partition the space."  After some bikeshed painting, So isClassOrInterface(), which means "not array, not primitive", seemed the best choice.   (The VM sees enums as classes and annotations as interfaces, and this is essentially a VM interop API.)  For value types, value types will be classes ("codes like a class!") so they will be included by this method. We could have call this isClassWhoseDescriptorStartsWithL() but that seemed a bit less friendly, though that's basically the intent.

it seems to be defined as !isPrimitive() and !isArray(), in that case, i think it can be removed.

Tried that, see above :)

EnumDesc and VarHandleDesc, those should be static inner classes of respectively Enum and VarHandle, they are not part of the main API but nice addons.

Good thought!  Will consider this (need to work through some of the other specializations we intend to add, to make sure this scales properly.)

DynamicCallSiteDesc is not a Desc (a ConstantDesc), so the name is very confusing.

It's not a ConstantDesc, but it _is_ a nominal descriptor.  Just not a nominal descriptor for a (loadable) constant.  The confusing bit is that its the only one of its kind, for now; later, I think this will become more common.  (If we had descriptors for the non-loadable constants in the CP (e.g., NameAndType), they'd be in this boat too.)

DynamicConstantDesc, it's not clear to me why this is not an interface like ClassDesc or MethodHandleDesc Fair question; we went back and forth on this a few times.  It could be shredded into a pair like ClassDesc/ConstantClassDesc.  However, in reality, we expect that all implementations of DCD would want to extend the base implementation anyway, since it provides useful functionality for which there's no point in reinventing.  Which means really DCD/ConstantDCD/AbstractDCD.  Which is starting to seem a little silly?   But I agree that things are a bit all over the map here.  (This is in part due to trying to cover the seam between primitive and reference class mirrors with a single abstraction, because the alternative is far worse.)

For ConstantClassDesc, ConstantMethodTypeDesc, ConstantMethodHandleDesc, do we really need to make these classes public, in theory they should not have a proper name because they have the same API as there interface counter parts and it will be easier to retrofit them as value type if there are not part of the public APIs. By doing that, i believe that FieldDescriptor and TypeDescriptor will not need to be generics anymore.

The reason these were made public is that we expect bytecode APIs to case over them.  If you're writing out an LDC based on a ConstantDesc, you can case over: String, Integer (and friends), Constant{Class,MT,MH}Desc, DynamicConstantDesc -- which are the concrete types that correspond exactly to the loadable CP types defined in JVMS -- and you're done.  If we made ConstantMHDesc private, the API gets more complicated, because now there are a bunch of methods that only work on direct method handles.

It seems that java.lang.invoke.Intrinsics has disappear (at least from the javadoc).

We split JEP 334 off from 303; Intrinsics remains in 303, and will come later.  This is the foundation.

Constable.describeConstable() is weirdly named, does asConstantDesc better ?

Yeah, we had quite a loooooong discussion on this...suffice it to say we considered every possible option, and then some.  The basic challenge is that ConstantDesc implements Constable, and code like     constantDesc.asConstantDesc() would totally look like a no-op -- but it's not!  It's asking the constantDesc to _describe itself_ as a constantDesc.  So calling it     constantDesc.describeConstable() is more clearly asking a Constable to describe itself.

Some implementation of Constable do not have the return type of describeConstable being tidy up (i.e. the return type uses the Optional<? ...> instead of a more specific one).

I'll take a look at these.  Some are trickier than they look.

I really dislike the fact that there is a method describeConstable in java.lang.Enum. Given the number of people that are interested by describeConstable(), i think this interface should be removed and static methods should be used instead. And fundamentally, i do not like the fact that you can go from a live object to its ConstantDesc representation, people will abuse of that like currently we use a java.lang.Class instead of a ClassDesc. This API, for me, goes in the wrong direction and pollute too many classes.

Yeah, this is a sharp edge.  But there's also a reason for it; it enables the compiler to much more aggressively constant-fold things into condy. The problem is not the method, or the mechanism -- the mechanism is awesome.  The problem is that this is a very low-level method bleeding into the Javadoc of some high-level things, which is sure to confuse users.  (We have the same problem with the two new methods on String -- describeConstable and resolveConstantDesc. Both are kind of silly on String, since they are basically identities.  In fact, the reason these methods are named so oddly is because they are going to show up in the API of some very general classes, like String.)

Hi all, I have uploaded another iteration of the implementation of the constants API + the javadoc. Thanks for the comments so far, Vicente [1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.09 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.09 On 05/24/2018 09:09 PM, Vicente Romero wrote:

Thanks for the comments so far, I have uploaded another iteration of the implementation + javadoc

Vicente

[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.08 [2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.08

On 05/23/2018 02:41 PM, Vicente Romero wrote:

...

Hi all,

Please review the proposed implementation for JEP 334 [1]. The webrev can be found at [2] and the javadoc at [3].

Thanks, Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8203252 [2] http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.pat... [3] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary...

PS. We are offering a MacBook Wheel to the authors of the first 5 comments :)