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

[ShinyaYoshida]

Hi all,
I've updated my patch, new webrev is here:
webrev.01.langtools:
http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.01.langtools/
webrev.01.jdk:
http://cr.openjdk.java.net/~shinyafox/kulla/8154714/webrev.01.jdk/ (Same as
previous)

In this version,
- rename /set add-exports to /set exports
- add /retain exports <module> <package>
- add tests
- move EXT_CMD_XXX into RemoteCodes
- make RemoteCodes public and move it into jdk.jshell.execution.code which
is module-private

Regards,
shinyafox(Shinya Yoshida)

2016-09-03 18:15 GMT+09:00 ShinyaYoshida <bitterfoxc at gmail.com>:

> 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.0
>>> 0.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)
>>>
>>
>>
>