----- Mail original ----- > De: "Paul Sandoz" <paul.san...@oracle.com> > À: "Remi Forax" <fo...@univ-mlv.fr> > Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>, "hotspot-dev" > <hotspot-...@openjdk.java.net> > Envoyé: Mardi 7 Novembre 2017 21:54:06 > Objet: Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants
> 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 <fo...@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? validateClassAccess. > > >> 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. the method returns a Class in its signature, so it can not be something else if not call by a condy and if the type is wrong in the bytecode, the VM will throw a CCE with the same info than your IAE. i still think it's unnecessary. > > >> 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. see my answer to John. > > >> 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. but you do that in invoke(). > > 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. Rémi
