RFR 8187742 Minimal set of bootstrap methods for dynamic constants

[Paul Sandoz]

Hi,

> On 9 Nov 2017, at 00:24, stanislav lukyanov <stanislav.lukyanov at oracle.com> wrote:
> 
> Hi Paul,
> 
> How about changing the name of the class to ConstantFactory to align
> with its two siblings - LambdaMetafactory and StringConcatFactory?
> 

I would prefer to keep it as is. It is intended to cover many forms of BSMs producing dynamic constants. LMF and SCF are very focused and there is some precedent that they may be called explicitly as factories where as for dynamic constants there is less motivation for this.


> Other comments are purely about the spec text (should I be posting them in the CSR issue instead?)
> 
> The class-level javadoc could use more text to describe the contents
> (e.g. "Methods to facilitate creation of common kinds of dynamic constants...", similar to other bootstrap factories)
> and motivation for having this class. I think giving the motivation here is important - it might be not obvious why, for example,
> nullConstant is needed when there is aconst_null.
> Maybe an example of an invokedynamic using static arguments that are dynamic constants (repeat the last six words together BTW!)
> could also be there (javap- or jasm-style pseudocode for that?)

I would prefer to follow up later on with more non-normative explanatory text. It will take some careful crafting and i don’t want that to side-track the review for the moment (including guidance on what forms of computed constants are acceptable).


> Also, @author

We don’t add @author.


> and @since are missing.
> 

Added.


> All methods repeat the same NPE condition
>     @throws NullPointerException if any used argument is {@code null}
> which looks especially strange in methods with one used parameter (e.g. nullConstant)
> or with no unused parameters (e.g. getStaticFinal).
> I'd suggest to instead go with the commonly used shortcut of declaring the NPE in the class-level Javadoc, e.g.
>     "Passing a null argument to a method in this class will cause of NullPointerException to be thrown,
>       unless the argument is specified to be unused, or unless otherwise noted”.


I like that:

* <p>The bootstrap methods in this class will throw a
* {@code NullPointerException} for any reference argument that is {@code null},
* unless the argument is specified to be unused or specified to accept a
* {@code null} value.

And adjusted the invoke method:

* @param args the arguments to pass to the method handle, as if with
* {@link MethodHandle#invokeWithArguments}.  Each argument may be
* {@code null}.
...
* @throws NullPointerException if {@code args} is {@code null}
* (each argument of {@code args} may be {@code null}).

We have been fleshing out the NPE behaviour because we know you will log bugs against us later on if we don’t :-)


> The VarHandle methods have
>     @param type unused; must be {@code Class<VarHandle>}
> Looks like the "unused;" part should be removed.
> 

Updated to:

  * @param type the required result type (must be {@code Class<VarHandle>})

and added the null check.


> On 9 Nov 2017, at 00:42, stanislav lukyanov <stanislav.lukyanov at oracle.com> wrote:
> 
> I also noticed that Class::isPrimitive now says the following:
>     "These objects may only be accessed via the following public static final variables, and are the only Class objects for which this method returns true."
> (the fields its talking about are the <Box>.TYPE)
> Should this text be tweaked?

I think it would be a distraction for most developers. The BSM is doing nothing special in regard to what isPrimitive specifies it just dynamically returns the value of the associated static field.

Paul.