Hi Paul,
just some quick process-related questions.
Is this intended to be targeted for jdk 10?
Is the current implementation already available in a separate
repository and/or branch? I've read in the RFR for 8186046 that some
parts are currently being refined in the amber repository. Or is the
complete implementation already available there?
If I want to port this to ppc64, what's the best way to start and what
do I need? Is it just the two webrevs for 8187742 and 8186046 on top
of the jdk hs repo? Or is it a branch of the amber repo?
You may know that there's currently a discussion going on about what
is a JEP and what's the right way to implement and integrate a JEP
into the mainline. The general idea is that only 'finished' JEPs will
be targeted and integrated into the always feature complete main line.
So is this and the review for 8186046 about the integration into the
jdk repo (which shouldn't happen before the JEP is not targeted for
jdk10) or is it just the review before the JEP can actually be
targeted? This is important because there's not much time before the
jdk10 repos will enter Rampdown phase 1 and we would still have to
port this to ppc (and also ARM/SPARC) if it will be targeted for jdk
10.
I don't want to question the merits and quality of the current
implementation which I'm sure are great. For me as an external
observer its just hard to oversee the current status of the
implementation.
Thanks,
Volker
On Thu, Nov 9, 2017 at 9:24 AM, stanislav lukyanov
<stanislav.lukya...@oracle.com> wrote:
> 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.
>>
>
