REPL tool implementation code review
Jan Lahoda
jan.lahoda at oracle.com
Mon Sep 7 09:52:12 UTC 2015
Hi,
Thanks for the review.
On 4.9.2015 20:46, Remi Forax wrote:
> Hi,
> just some comments on top of Paul's comments,
>
> On 09/04/2015 07:19 PM, Paul Sandoz wrote:
>> Hi,
>>
>> Here is the review of the REPL tool implementation.
>>
>> Generally it all looks very good. I ran out of time to analyse the tests more closely. Quite a few comments are suggestions related to using newer APIs or techniques.
>>
>> Paul.
>>
>> Implementation:
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:307
>>> byte[] encoded = Files.readAllBytes(Paths.get(filename));
>>> cmdlineStartup = new String(encoded);
>>
>>
>> What character set is the file encoded in? should the platform's default charset be used or UTF-8? Same applies to processing the load list files.
I'll leave the final answer on Robert, but I'd expect the platform's
default charset to be used - the file is provided by the user on the
command line, so I'd assume it is in the platform's encoding.
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:582
>>> int lastSlash = code.lastIndexOf('/'); //XXX
>>
>> Rogue comment.
Will fix.
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:587
>>> try (DirectoryStream<Path> dir = Files.newDirectoryStream(current)) {
>>> for (Path e : dir) {
>>> if (!accept.test(e) || !e.getFileName().toString().startsWith(prefix))
>>> continue;
>>> result.add(new Suggestion(e.getFileName() + (Files.isDirectory(e) ? "/" : ""), false));
>>> }
>>> } catch (IOException ex) {
>>> //ignore…
>>> }
>>
>> See files.lines(path) that returns a stream with map.filter.collect(toCollection(ArrayList::new))
Will fix.
>>
>>
>>> if (path.isEmpty()) {
>>> for (Path root : FileSystems.getDefault().getRootDirectories()) {
>>> if (!accept.test(root) || !root.toString().startsWith(prefix))
>>> continue;
>>> result.add(new Suggestion(root.toString(), false));
>>> }
>>> }
>>
>> Alas we don’t have a FileSystem.rootDirectories method that returns a stream.
>
> The array is two small to worth a dedicated stream method, no ?
> and one can use, Arrays.stream(...).
I'll use StreamSupport.stream to get the Stream.
>
>>
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:645
>>> registerCommand(new Command("/list", "/l", "[all]", "list the source you have typed",
>>> arg -> cmdList(arg),
>>> new FixedCompletionProvider("all")));
>>
>>
>> You could use a method reference for "arg -> …”, same applies for other command declarations.
Not all of the command actions can be converted to method references
currently (some of the methods don't need "arg"), so I'd slightly prefer
to keep the code consistent and keep the lambdas there. But, if method
refs would be strongly preferred, I can put them there.
>
> Also, it's a good idea to put the lambda as last argument so one can write,
>
> new Command (foo, bar, baz, arg -> {
> ...
> })
>
The current order is:
1. the command name (and help/description)
2. what the command does
3. the completion for the command
4. (optional) extra attributes
To me, this order seems reasonable (going from more important things to
less important things). So, I'd personally keep it, but I can change the
order if strongly preferred.
>>
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:716
>>> public List<Suggestion> commandCompletionSuggestions(String code, int cursor, int[] anchor) {
>>> code = code.substring(0, cursor);
>>> int space = code.indexOf(' ');
>>> List<Suggestion> result = new ArrayList<>();
>>>
>>> if (space == (-1)) {
>>> for (Command cmd : new LinkedHashSet<>(commands.values())) {
>>> if (cmd.kind == CommandKind.HIDDEN || cmd.kind == CommandKind.HELP_ONLY)
>>> continue;
>>> String key = cmd.aliases[0];
>>> if (key.startsWith(code)) {
>>> result.add(new Suggestion(key + " ", false));
>>> }
>>> }
>>> anchor[0] = 0;
>>> } else {
>>> String arg = code.substring(space + 1);
>>> String cmd = code.substring(0, space);
>>> Command command = commands.get(cmd);
>>> if (command != null) {
>>> result.addAll(command.completions.completionSuggestions(arg, cursor - space, anchor));
>>> anchor[0] += space + 1;
>>> }
>>> }
>>>
>>> Collections.sort(result, (s1, s2) -> s1.continuation.compareTo(s2.continuation));
>>> return result;
>>> }
>>
>>
>>
>> I believe you could use a stream here for both branches then do a sorted(…).collect(toList()) at the end of the method. For the first branch i think it might be something like values().distinct().filter(…).map(…).filter(…). For the second branch it is either an empty stream or the stream from the list.
Will fix.
>>
>> FYI: you can also to List.sort now.
>>
>>
[snip]
>> jdk/internal/jshell/tool/JShellTool.java:1710
>>> public ScannerIOContext(InputStream inStream, PrintStream pStream) {
>>> this(new Scanner(inStream), pStream);
>>> }
>>
>> Unused constructor.
Will fix.
[snip]
>>
>>> if (!key.pollEvents().isEmpty()) {
>>> if (!input.terminalEditorRunning()) {
>>> saveFile();
>>> }
>>> }
>>
>> Why does the watcher thread need to call saveFile when it is also performed after the process running the external editor has terminated? I am guessing every time the file is saved in the editor it is processed by the REPL?
Yes, I think the intent is that when the (GUI) editor saves the file, it
is immediately processed by REPL, so the user can see the outcomes
without quiting the editor.
Note this processing is disabled when the editor is detected to be a
terminal editor (as processing the input while the terminal editor is
running would make a mess of the screen).
[snip]
>> jdk/internal/jshell/tool/ConsoleIOContext.java:70
>>> TerminalFactory.registerFlavor(Flavor.WINDOWS, JShellWindowsTerminal :: new);
>>> TerminalFactory.registerFlavor(Flavor.UNIX, JShellUnixTerminal :: new);
>>
>> The syntax for the constructor refs is a little unusual with the white space. Same further down when binding CRTL_UP/DOWN actions.
Will fix.
>>
>>
>> jdk/internal/jshell/tool/ConsoleIOContext.java:136
>>> .forEach(s -> { result.add(s); });
>>
>> forEach(result::add);
Will fix.
>>
>>> Optional<String> prefix =
>>> suggestions.stream()
>>> .map(s -> s.continuation)
>>> .reduce((s1, s2) -> commonPrefix(s1, s2));
>>
>> .reduce(ConsoleIOContext::commonPrefix)
Will fix.
>>
>>
>>
>>> try {
>>> Method setBuffer = in.getClass().getDeclaredMethod("setBuffer", String.class);
>>>
>>> setBuffer.setAccessible(true);
>>> setBuffer.invoke(in, in.getHistory().current().toString());
>>> in.flush();
>>> } catch (ReflectiveOperationException | IOException ex) {
>>> throw new IllegalStateException(ex);
>>> }
>>
>> You might wanna comment more about the limitation in ConsoleReader and the necessity of calling a private method.
I'll add a comment there.
>>
>>
>>> private static String commonPrefix(String str1, String str2) {
>>> for (int i = 0; i < str2.length(); i++) {
>>> if (!str1.startsWith(str2.substring(0, i + 1))) {
>>> return str2.substring(0, i);
>>> }
>>> }
>>>
>>> return str2;
>>> }
>>
>> What we need is a String mismatch method, from which to build a common prefix method, stay tuned….
Ok.
>>
>>
>> jdk/internal/jshell/tool/ConsoleIOContext.java:505
>>> Set<Integer> snippetsStart = new HashSet<>();
>>> for (String start : prefs.get(HISTORY_SNIPPET_START, "").split(";")) {
>>> if (start.isEmpty())
>>> continue;
>>> snippetsStart.add(Integer.parseInt(start));
>>> }
>>
>> Could streamify but perhaps not worth it. At least convert to
>>
>> if(!start.isEmpty())
>> snippetsStart.add(Integer.parseInt(start));
Will fix.
>>
>>
>>> List<String> keys = new ArrayList<>(Arrays.asList(prefs.keys()));
>>> Collections.sort(keys);
>>
>> Stream.of(prefs.keys()).sorted().collect(toList());
Will fix.
>>
>>
>> jdk/internal/jshell/tool/ConsoleIOContext.java:
>>
>> I am struggling to understand the interactions of the before/afterUse code blocks, the CircularBuffer, and the thread created for writing (or pushing) read values to the buffer.
>>
>> CircularBuffer feels like a blocking queue, but there is some additional communication signalling how much input is needed.
This is a tricky code trying to resolve interaction between two of our
"special" features:
a) ability to stop/kill user's code by pressing Ctrl-C
b) run terminal editor in /edit (as opposed to GUI editors, which don't
need to use the terminal)
For "stop", we need to read from the input to detect the Ctrl-C - in
particular we need to read from the input while the user's code is
running to detect the stop request.
For terminal editors, we need to let the terminal editor work with the
terminal as it wants - in particular, we must not read from the input
(otherwise, not all of the user's input would reach the terminal editor).
A complicating aspect is that (to my knowledge), we can't do a
non-blocking read from System.in.
So the solution (not saying there can't be a better one) is to only read
from the input if we know we will process the input character:
-during ConsoleReader.readLine only if/when the ConsoleReader actually
requests to read from the input (because it will then process the
character) - see the InputStream returned from
ConsoleIOContext.InputHandler.wrapInIfNeeded.
-when the user's code is running (i.e. inside the
beforeUserCode-afterUserCode pair) - but, in this case, we only need to
process the Ctrl-C character (i.e. (int) 3) - we need to do something
with the rest, and the solution is to stash it into a buffer, so that it
is not lost and can be processed in the future.
This way we are not doing System.in.read() (or alike) while the terminal
editor is running, so that it gets the input characters that the user
types, but still can read as much as we need while the user's code is
running.
[snip]
>> CommandCompletionTest.java:154
>>> private List<String> listFiles(Path path, DirectoryStream.Filter<? super Path> filter) throws IOException {
>>> List<String> paths = new ArrayList<>();
>>> try (DirectoryStream<Path> stream = Files.newDirectoryStream(path, filter)) {
>>> stream.iterator().forEachRemaining(p -> {
>>> String fileName = p.getFileName().toString();
>>> if (Files.isDirectory(p)) {
>>> fileName += "/";
>>> }
>>> paths.add(fileName);
>>> });
>>> }
>>> Collections.sort(paths);
>>> return paths;
>>> }
>>
>>
>> See Files.list which returns a Stream<Path>, you could probably reformulate using Predicate rather than DirectoryStream.Filter.
Will fix.
Thanks,
Jan
>
> and try to do not use stream.iterator() which is usually slow, stream.forEach() is fine here.
>
>>
>>
>>
>> StartOptionTest.java:68
>>> private void start(Consumer<String> checkOutput, Consumer<String> checkError, String...args) throws Exception {
>>
>>
>> Space between … and parameter name. Same goes for other cases in this file.
>>
>
> cheers,
> Rémi
>
>
More information about the kulla-dev
mailing list