RFR: 8182297: jshell tool: pasting multiple lines of code truncated

[Jan Lahoda]

On 27.6.2017 21:04, Robert Field wrote:
> Style nit: in this class all field definitions are at the top of the
> class, which I believe is the coding standard.  This new field is two
> thousand lines down.  I understand that this field is private to
> readLine, but a comment could communicate that.

Agreed, moved down to the "//where:" section.

>
> Style question: adding to pushBackChars in the original is decidedly Old
> School (an old for-loop *gasp*), and the proposed is all streamy, but it
> is twice as long, involves more steps, seems harder to understand,
> involves intermediate collections, and is probably slower.  Is there a
> reason?

The reason for the intermediate collection is this: before, 
pushBackChars was always empty, so it was enough to just iterate through 
"prefix" in the reverse order and push the characters onto the stack. 
After this, the first input character was at the top of the stack, the 
last at the bottom, which ensured the correct order.

After making pushBackChars a field, the stack may already contain data 
from previous invocations of readLine. So, the content of "prefix" needs 
to be added in the reverse order at the bottom of the stack.

This could be done using:
for (int i = 0; i < prefix.length(); i++) {
     pushBackChar.add(0, prefix.charAt(i));
}

but each invocation of the add method would need shift all the content 
to make room for the new (single) char, and this could take a while. So, 
using the temporary list should ensure there will be at most one content 
shifting in pushBackChar.

In an updated webrev, I used a for instead of the stream 
manipulation&explicit reverse, might be slightly better.

Updated webrev for the jdk repository:
http://cr.openjdk.java.net/~jlahoda/8182297/jdk.01/

The webrev for the langtools repository remains the same:
http://cr.openjdk.java.net/~jlahoda/8182297/langtools.00/

Thanks,
     Jan

>
> Seems OK, address above at your discretion.
>
> -Robert
>
>
>
> On 06/27/17 11:14, Jan Lahoda wrote:
>> Hi,
>>
>> Currently, when pasting multiple lines into JShell, only the first two
>> lines are used.
>>
>> The cause is that when
>> jdk.internal.jline.console.ConsoleReader.readLine, we need to read the
>> cursor position. This is e.g. to avoid backspacing into the prompt. I
>> believe standard JLine is solving this by determining the length of
>> the prompt, but that does not work well for JShell, as the prompt is
>> not always known (e.g. when using System.in).
>>
>> So, there's a code to query the cursor position - this is done by
>> writing a query to the terminal and reading the answer from the
>> standard input.
>>
>> But if there's something in the standard input before the answer, it
>> needs to be read as well. So, this is read and stored in a
>> queue/stack, but the problem is that the stack is a local variable in
>> the readLine method. So only one line of the input is used and the
>> rest is dropped. The reason why two lines are used after paste is that
>> the above happens for the second pasted line; the measurements for the
>> first line are usually already done for the first line, which is not
>> so affected.
>>
>> The proposed solution is to store the todo stack in a field.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8182297
>>
>> Webrev:
>> jdk repository:
>> http://cr.openjdk.java.net/~jlahoda/8182297/jdk.00/
>> langtools repository (test):
>> http://cr.openjdk.java.net/~jlahoda/8182297/langtools.00/
>>
>> Thanks,
>>    Jan
>