RFR 8170162: jshell tool: no mechanism to programmatically launch

[Robert Field]

Thank you for the review, Jon!

Responses in-line below.

On 12/07/16 18:50, Jonathan Gibbons wrote:
> Many of the new files in the langtools webrev do not have the correct, 
> required, legal header. This is a MUST-FIX,

Oops!!

>
> In the new package-info file, this comment is a little bit hokey 
> and/or out of place.
>
>   52  * Note: The Java™ shell tool is built on the {@link 
> jdk.jshell} API,
>   53  * but the tool is at a different level, and this launching SPI 
> does not
>   54  * interact with the {@link jdk.jshell} API.

Not sure what is hokey, but the additional documentation I added to 
overview,html clarifies the relationship between the layers more clearly 
and is more appropriately placed than this, so, I will remove.
BTW: Seems I should dump overview.html, putting that in module-info.java?

>
> At a minimum, I suggest making it an @apiNote, but I would recommend 
> trying to rethink/reconsider
> what you are trying to say here.
>
> I think you should figure a way and place to specify the non-null name 
> used by the standard implementation
> of the JShellToolProvider. That being said ....

"The name of the standard implementation is 'jshell'" ???
In jdk.jshell.tool.JavaShellToolProvider.name()

>
> I guess I'm surprised you need to define a JShellToolProvider PAI -- 
> do you expect there to be multiple
> implementations of JShell on any system?

Maybe not.  But who knows, we already have an external fork..
I suppose we could have experimental versions in incubator modules...
I could also imagine a future with the jshell tool implementation broken 
into a separate module and slimmed-down runtime images built.

>   Why not just have
>     static JShellToolProvider.instance()
> to return an instance of the one true jshell implementation?

Certainly an option (depending on the above).  Though the difference in 
interface is:

     findFirst(String)
     name()

vs

     instance()

So, we get future-proofing almost for free.

The interfaces and implementation classes are separate, but I'd want 
this anyhow, since I very intentionally have them in separate packages.

-Robert


>
> -- Jon
>
>
> On 11/26/2016 02:19 PM, Robert Field wrote:
>> Goldilocks and the three launching mechanisms...
>>
>> She lay down in the first bed, but it was too hard.
>>     java.util.spi.ToolProvider (8169821) has a severe impedance 
>> mismatch: the use of Writers, no InputStream, and no flexibility.
>>
>> Then she lay in the second bed, but it was too soft.
>>     javax.tools.ToolProvider (8170044) would require significant 
>> changes that are not possible at this time, it is limited, and is old 
>> mechanism.
>>
>> Then she lay down in the third bed and it was just right. Goldilocks 
>> fell asleep.
>>     jdk.jshell.tool (8170162) provides a matching, clean, and 
>> configurable launching mechanism
>>
>> Please review...
>>
>> Bug:
>>
>>     8170162: jshell tool: no mechanism to programmatically launch
>>     https://bugs.openjdk.java.net/browse/JDK-8170162
>>
>> Sub-Tasks (tiny tweaks to other repos):
>>
>>     8170194: jshell tool (jdk repo): launch tool from JShellToolProvider
>>     https://bugs.openjdk.java.net/browse/JDK-8170194
>>
>>     8170195: jshell tool (make): update javadoc generation for 
>> jdk.jshell
>>     https://bugs.openjdk.java.net/browse/JDK-8170195
>>
>> Webrevs:
>>
>>     http://cr.openjdk.java.net/~rfield/8170162v0.webrev/
>>
>>     http://cr.openjdk.java.net/~rfield/8170194v0.webrev/
>>
>>     http://cr.openjdk.java.net/~rfield/8170195v0.webrev/
>>
>> API:
>>
>>     http://cr.openjdk.java.net/~rfield/8170162v0.jshellAPI/
>>
>> SpecDiff:
>>
>> http://cr.openjdk.java.net/~rfield/8170162v0.specdiff/overview-summary.html 
>>
>>
>> Thanks,
>> Robert
>>
>