REPL tool implementation code review

[Paul Sandoz]

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.


> 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!
> 

Thanks,
Paul.