REPL tool implementation code review

[Paul Sandoz]

On 10 Sep 2015, at 11:13, Jan Lahoda <jan.lahoda at oracle.com> wrote:

> Hi,
> On 7.9.2015 18:26, Paul Sandoz wrote:
>> On 7 Sep 2015, at 17:31, Jan Lahoda <jan.lahoda at oracle.com> wrote:
>>> I've tried to resolve some of the comments from the original e-mail here:
>>> http://hg.openjdk.java.net/kulla/dev/langtools/rev/45e459d45f64
>>> 
>> 
>> Looks good, it’s ok to do:
>> 
>>   forEach(results::add)
>> 
>> rather than create additional collections if you know what you are doing.
> 
> Done here:
> 
> http://hg.openjdk.java.net/kulla/dev/langtools/rev/dbbedddaf7fe
> 

+1


>> 
>> 
>>> On 7.9.2015 14:59, Paul Sandoz wrote:
>>>> On 7 Sep 2015, at 11:52, Jan Lahoda <jan.lahoda at oracle.com> wrote:
>>>> 
>>>> Is the asynchronous operation on the REPL in 4 an issue?
>>> 
>>> The asynchronous operation is also somewhat tricky (but necessary, I think). I have another patch in my queue that reflects (some of) Brian comments on that.
>>> 
>> 
>> Ok, i was more questioning whether it is safe rather than necessary to perform on the reading thread, since i could not determine if stopping the REPL concurrently in the reader thread would interfere with executing user code performed in the main thread.
>> 
>> 
>>>> 
>>>> My inclination is you should try and merge the thread logic in InputBuffer.wrapInIfNeeded into CircularBuffer, then modify and rename CircularBuffer to extend from InputStream, thereby containing all the relevant multi-threaded logic and it becomes an obvious thing being deferred to. You could also add another state CLOSED, which i think more clearly indicates closure rather than continually writing -1 every time you read -1.
>>>> 
>>>> Sorry for labouring the point here, but i think this kind of thing is worth spending some time on. Consider someone trying to modify the code in a few years time. Review-wise we could spin this off as a separate bug so as not to block the code going in.
>>> 
>>> No problem. I'll look into doing the changes as you describe. Thanks for the comments!
> 
> I've done some improvements to the CircularBuffer (including renaming it) here:
> http://hg.openjdk.java.net/kulla/dev/langtools/rev/4141dc296d4d
> 
> I've also cleaned up the related code a little in the change, to simplify how the ConsoleIOContext works.
> 
> How does it look?
> 

This looks much better! StopDetectingInput is now a nicely contained piece of logic.

Perhaps StopDetectingInput should be renamed to StopDetectingInputStream?

In ConsoleIOContext you do:

if (System.getProperty("os.name").toLowerCase(Locale.US).contains(TerminalFactory.WINDOWS)) {
    term = new JShellWindowsTerminal(this);
} else {
    term = new JShellUnixTerminal(this);
}

You are passing a partially constructed instance of ConsoleIOContext as an argument to the constructor of the terminal-based classes. Perhaps you can just pass the StopDetectingInput instance instead?

Paul.