RFR 8168615: JShell API: jdk.jshell.spi should be a pluggable ServiceLoader SPI

[Robert Field]

Thanks for the review, Jan.

See responses inline.

On 12/06/16 10:14, Jan Lahoda wrote:
> A few comments:
> -the -X help will (in my understanding) produce this:
> +\    --execution <spec>                 Specify an alternate 
> execution engine.\n\
> +\                                        Where <spec> is an 
> ExecutionControldProvider spec.\n\
> +\                                        See jdk.jshell.spi\n\
>
> As this is a user-level help (although a very experienced user, 
> presumably), it would be better I think to provide some details - e.g. 
> the grammar for the spec; known provider; and ideally, parameters of 
> the known providers

OK, so better doc references are in order.  I think including the 
grammar and explanatory information is probably too bulky for 
command-line help.  But I've clarified this help as follows (the 
formatting is right if fix-width, but formatted text cannot be sent to 
this list):

          % jshell -X
              --add-exports <module>/<package>   Export specified 
module-private package to snippets
              --execution <spec>                 Specify an alternate 
execution engine.
                                                   Where <spec> is an 
ExecutionControl spec.
                                                   See the documentation 
of the package
                                                   jdk.jshell.spi for 
the syntax of the spec

          These options are non-standard and subject to change without 
notice.

Where the jdk.jshell.spi javadocs are now:

http://cr.openjdk.java.net/~rfield/8168615v1.jshellAPI/jdk/jshell/spi/package-summary.html

It is OK that they would need to reference the SPI docs because if they 
implemented an execution engine they would have implemented these 
interfaces,  If they are just tweaking parameters (and this isn't 
something we want a typical user to do) they would need to be familiar 
with the parameters, which are described in these docs.


>
> -I would reorganize the rules in the spec grammar to go from the top 
> down:
> +     * spec := name : params
> +     *       | name
> +     * name := id
> +     * params := param , params
> +     *        | param
> +     *        |
> +     * param := id ( value )
>
> (might be also good to say what is "id")

Both good ideas implemented in the above.

>
> -ExecutionControlProvider.logMessages seems weird. Is it used outside 
> of tests?

Not currently, but we really need logging of failures and failover to 
track-down rare intermittent issues.  Adding logging is out of scope for 
9, but this gives a needed hook.

>
> -after this patch, both the ExecutionControls and their Providers are 
> public - is that desirable? Is there a usecase for the client to 
> call/instantiate the Providers directly without going through the 
> ServiceLoader?

The ExecutionControls were public before.  jdk.jshell.execution contains 
the default execution engines, but it is exposed because it is a library 
of classes for implementing ExecutionControl (and now 
ExecutionControlProvider).

The SPI and the jdk.jshell.execution library were created because of a 
request from an IDE developer.

OK, here is the new version.  No executable code was changed, but I've 
spruced-up the documentation, including what is mentioned above.

The docs:

     http://cr.openjdk.java.net/~rfield/8168615v1.jshellAPI/

Specdiff:

http://cr.openjdk.java.net/~rfield/8168615v1.specdiff/overview-summary.html

New webrev:

     http://cr.openjdk.java.net/~rfield/8168615v1.webrev/

-Robert

>
> Jan
>
> On 2.12.2016 08:48, Robert Field wrote:
>> Please review...
>>
>> Bug:
>>
>>       https://bugs.openjdk.java.net/browse/JDK-8168615
>>
>>
>> Webrev:
>>
>>      http://cr.openjdk.java.net/~rfield/8168615v0.webrev/
>>
>> Thanks,
>> Robert
>>