REPL tool implementation code review

[Jan Lahoda]

On 10.9.2015 18:14, Paul Sandoz wrote:
>
> On 10 Sep 2015, at 11:13, Jan Lahoda <jan.lahoda at oracle.com> wrote:
>>>> 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?

Right, sorry for that.

Fixed:
http://hg.openjdk.java.net/kulla/dev/langtools/rev/d76df662d99d

Thanks,
     Jan

>
> Paul.
>