WIP: 8154714: JShell API: addExports support [for Java 9.X]

[ShinyaYoshida]

Hi Robert,
Thank you for reviewing.
I'll update my patch later.

2016-09-02 11:38 GMT+09:00 Robert Field <robert.field at oracle.com>:

> Thanks shiyafox! This is a significant piece of code!
>
> As to your questions --
>
> > I wonder if  you could work on the documentation of the API and the tool
> instead?
>
> Yes, I will do API and tool docs.

Thank you!

> Subcommand of /set is reasonable? Similar command, /classpath, to make
> users available to use new packages is not a subcommand of /set.
>
> Yes, I think /set exports is best.  And, yes, /classpath is inconsistent,
> it should be /set classpath, that can be a separate task, unless you want
> to roll that in.

I see, I've created the issue to move /classpath to /set classpath:
https://bugs.openjdk.java.net/browse/JDK-8165405


> > Should we need retain feature to this?
>
> All /set have a /retain counterpart -- we should maintain that
>
Ok, I'll add /retain exports.
I've also create the issue for retaining classpath:
https://bugs.openjdk.java.net/browse/JDK-8165406


> As to the code review: Looks good to me, just minor questions/issues --
>
> JShellTool.java:
>
> * As discussed /set add-exports => /set exports
>
> * Nit: a comment got moved out of place line 1194(old)
>
> JShell.java:
>
> * EXT_CMD_AVAILABLE_MODULES and EXT_CMD_AVAILABLE_PACKAGES, these should
> be referenced via RemoteCodes.java rather than be coped.
>
Absolutely! I'd like to move these into RemoteCodes but it is
package-private class and cannot be referred from JShell.
Can I make RemoteCodes public and move it to another module-private package
like jdk.jshell.execution.code?


> * Which leads me to another question: why is addExports a command and
> these two are extensions.  It seems fine, just curious.  I notice that
> extensions are a lot less code -- ;-)

Exporting module and package is similar to adding classpath to platform, so
I made the addExports a command like setClasspath.
And it should be added as API because it is very basic feature.


> TaskFactory.java:
>
> * Question: the "/" separator between module and package is that platform
> independent?
>
It seems to me that "{module}/{path}" format for --add-exports option of
javac is platform independent.

Regards,
shinyafox(Shinya Yoshida)


> DefaultLoaderDelegate.java:
>
> * Jan, can you look at this code?
>
>
> -Robert
>
>
> On 09/01/16 05:36, ShinyaYoshida wrote:
>
>> Hi all,
>> I've implemented this feature to JShell API and Tool with full completion:
>> Bugs: https://bugs.openjdk.java.net/browse/JDK-8154714
>>
>> jdk: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.00.jdk/ <
>> http://cr.openjdk.java.net/%7Eshinyafox/kulla/8154714/webrev.00.jdk/>
>> langtools: http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.
>> 00.langtools/ <http://cr.openjdk.java.net/%7
>> Eshinyafox/kulla/8154714/webrev.00.langtools/>
>>
>> This is missing the train of Java 9.0 but it could catch the train of
>> Java 9.1 or later.
>> I want to get your comments for this.
>>
>> jshell> /set add-exports jdk.jconsole sun.tools.jconsole
>>
>> jshell> import sun.tools.jconsole.*
>>
>> jshell> LocalVirtualMachine.getAllVirtualMachines().forEach((k, v) ->
>> System.out.println(k + ":" + v))
>> 10778:jdk.jshell/jdk.internal.jshell.tool.JShellTool
>> 10826:jdk.jshell.execution.RemoteExecutionControl 35548
>> 28447:org.netbeans.Main --cachedir /home/bitter_fox/.cache/netbeans/8.0.1
>> --userdir /home/bitter_fox/.netbeans/8.0.1 --branding nb
>>
>> Currently, there is no testing, so it will be required to add test.
>>
>> Regards,
>> shinyafox(Shinya Yoshida)
>>
>
>