Re: RFR: JDK-8214491: Upgrade to JLine 3.9.0
CC'ing nashorn-dev. -Sundar On 05/12/18, 10:48 PM, Jan Lahoda wrote: Hi Robert, On 4.12.2018 23:59, Robert Field wrote: I saw no issues with JShell tool and test portions of the webrev. I did not review the nashorn changes. Thanks for looking at this! Testing it: editing multi-line snippets is vastly easier and more intuitive. There is one issue, with the old mechanism, as horridly clunky as it was, you could add new lines of code (a frequently needed functionality). Since is accept, I could find no way to add lines with this JLine 3 version. Thoughts? One possibility that comes to mind: pressing Enter inside the snippet would add a new line, Enter at the very end of a (complete) snippet would confirm that snippet. Could be fairly convenient/intuitive. What do you think? I looked into readline commands as an approach to addressing this, found nothing. However, most readline commands worked. Ctrl-u however did not behave as documented in readline (instead deleting to the beginning of the line). I notice that the there is zero in-command documentation of command-line editing -- not even a mention. Independent from the review of this port, it seems we should have in-command documentation of command editing -- even more so now that multiline editing is useful. This is about enhancing /help, right? I'll see what I can do. The JShell User Manual will also need a little edit in the History Navigation section. Where is that? Thanks, Jan -Robert On 11/29/18 1:06 PM, Jan Lahoda wrote: Hi, I'd like to update the internal JLine used by JShell and jjs to JLine 3.9.0. Two notable advantages of this version is multi-line snippet editing and better UI on Windows (escape sequence support on Windows). As a consequence, this patch drops EditingHistory, as it does not seem to be needed anymore. JBS: https://bugs.openjdk.java.net/browse/JDK-8214491 The full patch is here: http://cr.openjdk.java.net/~jlahoda/8214491/webrev.00/ To make the changes more clear, I've split it into two: -replacement of existing JLine with the new on in jdk.internal.le, no changes to JLine code: http://cr.openjdk.java.net/~jlahoda/8214491/webrev.00.replace/ -tweaks to JLine (repackaging, eliminating references to j.u.l.Logger, adding hooks to wrap input streams with our stop-detecting input stream, adding unicode escapes to unicode characters, support for Windows without JNA), adjustments to JShell and jjs (unfortunately, 3.9.0 is not compatible with the JLine 2 branch, so the changes are substantial): http://cr.openjdk.java.net/~jlahoda/8214491/webrev.00.update/ Any feedback is welcome! Thanks, Jan
Re: RFR: 8214795: Add safety check to dynalink inner class lookup
+1 > On 2018. Dec 5., at 15:33, Hannes Wallnöfer > wrote: > > Please review: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214795 > Webrev: http://cr.openjdk.java.net/~hannesw/8214795/webrev.00/ > > This is to make sure we use the right inner classes regardless of the order > of classes returned by Class.getClasses(). > > Thanks, > Hannes
Re: RFR: 8214795: Add safety check to dynalink inner class lookup
This code is ultimately invoked from BeanLinker constructor, so always on a single thread; there’s no race here. putIfAbsent was used here previously solely for its effect of not replacing existing mappings, not because of its atomicity. Attila. > On 2018. Dec 5., at 15:45, Jim Laskey wrote: > > Wouldn’t you still use innerClasses.putIfAbsent in case there is a race? > >> On Dec 5, 2018, at 10:33 AM, Hannes Wallnöfer >> wrote: >> >> Please review: >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8214795 >> Webrev: http://cr.openjdk.java.net/~hannesw/8214795/webrev.00/ >> >> This is to make sure we use the right inner classes regardless of the order >> of classes returned by Class.getClasses(). >> >> Thanks, >> Hannes >
Re: RFR: 8214795: Add safety check to dynalink inner class lookup
Wouldn’t you still use innerClasses.putIfAbsent in case there is a race? > On Dec 5, 2018, at 10:33 AM, Hannes Wallnöfer > wrote: > > Please review: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214795 > Webrev: http://cr.openjdk.java.net/~hannesw/8214795/webrev.00/ > > This is to make sure we use the right inner classes regardless of the order > of classes returned by Class.getClasses(). > > Thanks, > Hannes
Re: RFR: 8214795: Add safety check to dynalink inner class lookup
Looks good. -Sundar On 05/12/18, 8:03 PM, Hannes Wallnöfer wrote: Please review: Bug: https://bugs.openjdk.java.net/browse/JDK-8214795 Webrev: http://cr.openjdk.java.net/~hannesw/8214795/webrev.00/ This is to make sure we use the right inner classes regardless of the order of classes returned by Class.getClasses(). Thanks, Hannes
Re: RFR: 8214795: Add safety check to dynalink inner class lookup
Looks good -Sundar On 05/12/18, 8:03 PM, Hannes Wallnöfer wrote: Please review: Bug: https://bugs.openjdk.java.net/browse/JDK-8214795 Webrev: http://cr.openjdk.java.net/~hannesw/8214795/webrev.00/ This is to make sure we use the right inner classes regardless of the order of classes returned by Class.getClasses(). Thanks, Hannes
RFR: 8214795: Add safety check to dynalink inner class lookup
Please review: Bug: https://bugs.openjdk.java.net/browse/JDK-8214795 Webrev: http://cr.openjdk.java.net/~hannesw/8214795/webrev.00/ This is to make sure we use the right inner classes regardless of the order of classes returned by Class.getClasses(). Thanks, Hannes
