RFR 8156101: JShell SPI: Provide a pluggable execution control SPI

[Jan Lahoda]

Seems OK to me.

Jan

On 16.5.2016 21:41, Robert Field wrote:
> Per Jan's review, I have updated the webrev:
>
>      http://cr.openjdk.java.net/~rfield/8156101v3_lang.webrev/
>
> Specifically:
> * SPIConstants has been removed (the values moved internal, to Util, and
> made package private).
> * The method name is now passed to ExecutionControl.invoke()  -- and
> correspondingly, is now sent over the wire to the RemoteAgent.
> * The expunge() method (and calls to it) have been removed from
> JDIExecutionControl and LocalExecutionControl.
> * SPIDebugControl has been removed (and InternalDebugControl un-removed,
> but now having the updated guts of SPIDebugControl).
> * To rid ourselves of InternalDebugControl in the future, I have
> created: https://bugs.openjdk.java.net/browse/JDK-8157080.
> * I have updated LocalExecutionControl to Grigory's latest version
> (which no longer has commented out code).
> * I have fixed the javadoc of ClassTracker.ClassInfo.
>
> Bug:
>      https://bugs.openjdk.java.net/browse/JDK-8156101
>
> -Robert
>
> On 05/16/2016 12:14 AM, Jan Lahoda wrote:
>> Overall, seems reasonable to me. Would be good to get a confirmation
>> from the clients, that it matches their needs. For the top-level
>> changes, a review from build-dev would be good.
>>
>> Comments:
>> -I am not sure of SPIConstants - is there a chance we could avoid this
>> class? I see that:
>> --DOIT_METHOD_NAME is used in invoke - maybe invoke could take the
>> method name as a parameter?
>> --PREFIX_PATTERN is used in implementation of
>> ExecutionControl.varValue; but do we want everybody to re-implement
>> the Object->String conversion? I was thinking if a method like:
>> ExecutionControl:
>> public static String toString(Object value) {...}
>> that every ExecutionControl implementation would call wouldn't work
>> better.
>>
>> -also not sure about SPIDebugControl - is there a chance we could
>> replace that with a group of Loggers, or something similar?
>>
>> -in KullaTesting I wonder if we could avoid the code duplication and
>> let the setUp() method delegate to setUp(ExecutionControl)
>>
>> A few nits:
>> -commented-out code in LocalExecutionControl.java
>> -javadoc of ClassTracker.ClassInfo:
>> Associates a class name class bytes and ReferenceType.
>> maybe a missing comma between "class name" and "class bytes"?
>>
>> Jan
>>
>> On 14.5.2016 05:55, Robert Field wrote:
>>> Updated langtools webrev with additional code comments only (to ease
>>> review).  No changes to code:
>>>
>>>      http://cr.openjdk.java.net/~rfield/8156101v2_lang.webrev/
>>>
>>> -Robert
>>>
>>>
>>>
>>> On 05/13/2016 12:29 AM, Robert Field wrote:
>>>> Updated langtools webrev:
>>>>     http://cr.openjdk.java.net/~rfield/8156101v1_lang.webrev/
>>>>
>>>> -Robert
>>>>
>>>>
>>>> On 05/12/2016 11:41 PM, Robert Field wrote:
>>>>> Attached is a test for this functionality utilizing a local
>>>>> implementation of ExecutionControl courtesy of Grigory Ptashko.
>>>>> Thank you Grigory!
>>>>>
>>>>> All files in test/jdk/jshell.
>>>>>
>>>>> This test needs one method added to KullaTesting --
>>>>>
>>>>> diff -r c51b40933e0c test/jdk/jshell/KullaTesting.java
>>>>> --- a/test/jdk/jshell/KullaTesting.java    Wed May 11 20:28:22 2016
>>>>> +0000
>>>>> +++ b/test/jdk/jshell/KullaTesting.java    Thu May 12 23:31:14 2016
>>>>> -0700
>>>>> @@ -72,6 +72,7 @@
>>>>>  import static jdk.jshell.Snippet.Status.*;
>>>>>  import static org.testng.Assert.*;
>>>>>  import static jdk.jshell.Snippet.SubKind.METHOD_SUBKIND;
>>>>> +import jdk.jshell.spi.ExecutionControl;
>>>>>
>>>>>  public class KullaTesting {
>>>>>
>>>>> @@ -166,6 +167,21 @@
>>>>>          classpath = new ArrayList<>();
>>>>>      }
>>>>>
>>>>> +    public void setUp(ExecutionControl ec) {
>>>>> +        inStream = new TestingInputStream();
>>>>> +        outStream = new ByteArrayOutputStream();
>>>>> +        errStream = new ByteArrayOutputStream();
>>>>> +        state = JShell.builder()
>>>>> +                .executionEngine(ec)
>>>>> +                .in(inStream)
>>>>> +                .out(new PrintStream(outStream))
>>>>> +                .err(new PrintStream(errStream))
>>>>> +                .build();
>>>>> +        allSnippets = new LinkedHashSet<>();
>>>>> +        idToSnippet = new LinkedHashMap<>();
>>>>> +        classpath = new ArrayList<>();
>>>>> +    }
>>>>> +
>>>>>      @AfterMethod
>>>>>      public void tearDown() {
>>>>>          if (state != null) state.close();
>>>>>
>>>>>
>>>>> On 05/11/2016 10:53 PM, Robert Field wrote:
>>>>>> Please review the new Service Provider Interface for using alternate
>>>>>> execution implements.
>>>>>>
>>>>>> Bug:
>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8156101
>>>>>>
>>>>>> Webrevs:
>>>>>> http://cr.openjdk.java.net/~rfield/8156101v0_lang.webrev/
>>>>>> http://cr.openjdk.java.net/~rfield/8156101v0_ws.webrev/
>>>>>>
>>>>>> Javadoc:
>>>>>>     http://cr.openjdk.java.net/~rfield/jshell_spi/
>>>>>>
>>>>>> Note: The SPI is tested since it is used by the default JDI
>>>>>> implementation (which has no special access).  However, tests using
>>>>>> an alternative execution engine will be included.
>>>>>>
>>>>>> Thanks,
>>>>>> Robert
>>>>>>
>>>>>
>>>>
>>>
>