Hi all, I've not taken a look to the code, so this is just my comments based on the javadoc.
This version is far better than the previous one, given that all my other comments are "you should change that too ..., i don't like ..., etc", i want to first say that i'm very please by this new version. MethodHandleDesc.Kind: The JVMS defines names for these constants, why using different names here ? ClassDesc.isClassOrInterface() seems very specific, does it includes annotation or enums, how will it works when value types will be included ? it seems to be defined as !isPrimitive() and !isArray(), in that case, i think it can be removed. 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. DynamicCallSiteDesc is not a Desc (a ConstantDesc), so the name is very confusing. ConstantDescs is a bag of constants, so if you want to use one, all of them need to be initialized, here we should own our own dog food and initialize these constants only when needed, so instead of being static final, they should be exposed as methods and implemented as a ldc on a ConstandDynamic. DynamicConstantDesc, it's not clear to me why this is not an interface like ClassDesc or MethodHandleDesc 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. It seems that java.lang.invoke.Intrinsics has disappear (at least from the javadoc). Constable.describeConstable() is weirdly named, does asConstantDesc better ? 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 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. For javadoc for java.lang shows too many classes, not only the one that are impacted by the JEP 334. regards, Rémi ----- Mail original ----- > De: "Vicente Romero" <vicente.rom...@oracle.com> > À: "amber-dev" <amber-...@openjdk.java.net>, "core-libs-dev" > <core-libs-dev@openjdk.java.net> > Envoyé: Mercredi 23 Mai 2018 20:41:11 > Objet: RFR: implementation for JEP 334: JVM Constants API > 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 :)
