RFR 8187742 Minimal set of bootstrap methods for dynamic constants

[stanislav lukyanov]

Hi Paul,

How about changing the name of the class to ConstantFactory to align
with its two siblings - LambdaMetafactory and StringConcatFactory?

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?)
Also, @author and @since are missing.

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".

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

Thanks,
Stas

On 08.11.2017 0:08, Paul Sandoz wrote:
> Hi,
>
> Please review the patch that adds a minimal set of bootstrap methods can be be used for producing dynamic constants:
>
>    http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/>
>
> The aim is to provide a small but common set of BSMs that are likely useful with ldc or as BSM arguments, filling in the gaps for constants that cannot be currently represented, such as null, primitive classes and VarHandles. It’s possible to get more sophisticated regarding say factories but that is something to consider later on.
>
> This patch is based off the minimal dynamic constant support (still in review):
>
>    http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html <http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html>
>
> Paul.
>