REPL tool implementation code review
Robert Field
robert.field at oracle.com
Fri Sep 4 19:10:19 UTC 2015
Thank you Paul, and Rémi!
-Robert
> On Sep 4, 2015, at 11:46 AM, Remi Forax <forax at univ-mlv.fr> 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:91
>>> private static final String VERSION = "0.819”;
>>> …
>>> case "-version":
>>> cmdout.printf("jshell %s\n", VERSION);
>>> return null;
>>>
>>
>> Should this version be the JDK version, as is the case for other Java tools?
>
> i agree.
>
>>
>> jdk/internal/jshell/tool/JShellTool.java:128
>>> private IOContext input = null;
>>> private boolean regenerateOnDeath = true;
>>> private boolean live = false;
>>>
>>> SourceCodeAnalysis analysis;
>>> JShell state = null;
>>> Subscription shutdownSubscription = null;
>>>
>>> private boolean debug = false;
>>> private boolean displayPrompt = true;
>>> public boolean testPrompt = false;
>>> private Feedback feedback = Feedback.Default;
>>> private String cmdlineClasspath = null;
>>> private String cmdlineStartup = null;
>>> private String editor = null;
>>>
>>
>> A small style thing, there is no need to initialize to default values (it’s also applied inconsistently)
>>
>> 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.
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:582
>>> int lastSlash = code.lastIndexOf('/'); //XXX
>>
>> Rogue comment.
>>
>>
>> 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))
>>
>>
>>> 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(...).
>
>>
>>
>>
>> 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.
>
> Also, it's a good idea to put the lambda as last argument so one can write,
>
> new Command (foo, bar, baz, arg -> {
> ...
> })
>
>>
>>
>>
>> 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.
>>
>> FYI: you can also to List.sort now.
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:763
>>> if (arg == null || arg.isEmpty()) {
>>
>> I dunno if it is possible but it would be nice to normalise to empty strings rather than checking for null or an empty string.
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:959
>>> Iterator<Snippet> it = keySet.iterator();
>>> while (it.hasNext()) {
>>> if (!(it.next() instanceof PersistentSnippet)) {
>>> it.remove();
>>> }
>>> }
>>
>> See the bulk operation Collection.removeIf.
>>
>>
>>> PersistentSnippet key = (PersistentSnippet) keySet.iterator().next();
>>> state.drop(key).forEach(e -> handleEvent(e));
>>
>>
>> For kicks you could also do it like this (not suggesting you actually do unless you really want to, it’s just instructive for new APIs in Java 8):
>>
>> keySet.stream().findFirst().
>> map(PersistentSnippet.class::cast).
>> ifPresent(k -> state.drop(k).forEach(this::handleEvent));
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:981
>>> private void cmdEdit(String arg) {
>>> Set<Snippet> keySet = argToKeySet(arg);
>>> if (keySet == null) {
>>> return;
>>> }
>>> Set<String> srcSet = new LinkedHashSet<>();
>>> for (Snippet key : keySet) {
>>> String src = key.source();
>>> switch (key.subKind()) {
>>> case VAR_VALUE_SUBKIND:
>>> break;
>>> case ASSIGNMENT_SUBKIND:
>>> case OTHER_EXPRESSION_SUBKIND:
>>> case TEMP_VAR_EXPRESSION_SUBKIND:
>>> if (!src.endsWith(";")) {
>>> src = src + ";";
>>> }
>>> srcSet.add(src);
>>> break;
>>> default:
>>> srcSet.add(src);
>>> break;
>>> }
>>> }
>>> StringBuilder sb = new StringBuilder();
>>> for (String s : srcSet) {
>>> sb.append(s);
>>> sb.append('\n');
>>> }
>>> String src = sb.toString();
>>
>>
>> There appears to be a reliance on an undefined iteration order, perhaps argToKeySet should return LinkedHashSet?
>>
>> I think this might be convertible to a stream with filter(…).map(…).collect(joining(“\n”, “”, “\n"))
>
> you need to use distinct() between map() and collect().
>
>>
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:1150
>>> try (BufferedWriter writer = new BufferedWriter(new FileWriter(resolveUserHome(filename)))) {
>>
>> See also Files.newBufferedWriter(…) i like that because it makes it more explicit whether you are appending to an existing file or not.
>
> and more correct because with "new BufferedWriter(new FileWriter" if the constructor of BufferedWriter raises an exception, the Filewriter will not be closed :(
>>
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:1266
>>> List<Diagnostic<String>> errorsOnly(List<Diagnostic<String>> diagnostics) {
>>> List<Diagnostic<String>> errors = new ArrayList<>();
>>> for (Diagnostic<String> diag : diagnostics) {
>>> switch (diag.getKind()) {
>>> case ERROR:
>>> errors.add(diag);
>>> break;
>>> case MANDATORY_WARNING:
>>> case WARNING:
>>> break;
>>> case NOTE:
>>> case OTHER:
>>> break;
>>> }
>>> }
>>> return errors;
>>> }
>>
>> diagnostics.stream().filter(d -> d.getKind() == ERROR).collect(toList());
>
> and instead of having a method that takes a List and returns a List, it's better to have
> a method that take a Stream and returns a Stream to avoid to create intermediary collections.
>
>>
>>
>>
>> jdk/internal/jshell/tool/JShellTool.java:1710
>>> public ScannerIOContext(InputStream inStream, PrintStream pStream) {
>>> this(new Scanner(inStream), pStream);
>>> }
>>
>> Unused constructor.
>>
>>
>> jdk/internal/jshell/tool/ExternalEditor.java:134
>>> private void saveFile() {
>>> List<String> lines;
>>> try {
>>> lines = Files.readAllLines(tmpfile);
>>> } catch (IOException ex) {
>>> errorHandler.accept("Failure read edit file: " + ex.getMessage());
>>> return ;
>>> }
>>> StringBuilder sb = new StringBuilder();
>>> for (String ln : lines) {
>>> sb.append(ln);
>>> sb.append('\n');
>>> }
>>> saveHandler.accept(sb.toString());
>>> }
>>
>> Why don’t you just read as bytes then create a String from those bytes?
>
> or Files.lines(tmpFile).collect(Collectors.joining("\n"))
>
>>
>>
>>> 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?
>>
>>
>> jdk/internal/jshell/tool/EditPad.java:46
>>> public class EditPad extends JFrame implements Runnable {
>>> private final Consumer<String> errorHandler;
>>> private final String initialText;
>>> private final boolean[] closeLock;
>>> private final Consumer<String> saveHandler;
>>>
>>> EditPad(Consumer<String> errorHandler, String initialText,
>>> boolean[] closeLock, Consumer<String> saveHandler) {
>>> super("Edit Pad (Experimental)");
>>> this.errorHandler = errorHandler;
>>> this.initialText = initialText;
>>> this.closeLock = closeLock;
>>> this.saveHandler = saveHandler;
>>> }
>>
>>
>> Unused field errorHandle.
>>
>> Is this still experimental? Is so should be it included?
>>
>>
>> jdk/internal/jshell/tool/EditPad.java:114
>>> private void notifyClose() {
>>> synchronized (closeLock) {
>>> closeLock[0] = true;
>>> closeLock.notify();
>>> }
>>> }
>>>
>>> public static void edit(Consumer<String> errorHandler, String initialText,
>>> Consumer<String> saveHandler) {
>>> boolean[] closeLock = new boolean[1];
>>> SwingUtilities.invokeLater(
>>> new EditPad(errorHandler, initialText, closeLock, saveHandler));
>>> synchronized (closeLock) {
>>> while (!closeLock[0]) {
>>> try {
>>> closeLock.wait();
>>> } catch (InterruptedException ex) {
>>> // ignore and loop
>>> }
>>> }
>>> }
>>> }
>>
>> You could use a shared CountDownLatch, rather than notify/wait/synchronization with a boolean array of one element.
>>
>>
>> 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.
>>
>>
>> jdk/internal/jshell/tool/ConsoleIOContext.java:136
>>> .forEach(s -> { result.add(s); });
>>
>> forEach(result::add);
>>
>>> Optional<String> prefix =
>>> suggestions.stream()
>>> .map(s -> s.continuation)
>>> .reduce((s1, s2) -> commonPrefix(s1, s2));
>>
>> .reduce(ConsoleIOContext::commonPrefix)
>>
>>
>>
>>> 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.
>>
>>
>>> 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….
>>
>>
>> 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));
>>
>>
>>> List<String> keys = new ArrayList<>(Arrays.asList(prefs.keys()));
>>> Collections.sort(keys);
>>
>> Stream.of(prefs.keys()).sorted().collect(toList());
>>
>>
>> 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.
>>
>>
>>
>> Tests
>>
>> General point:
>>
>> (a) -> …
>>
>> Can be rewritten as:
>>
>> a -> ...
>>
>>
>> ReplToolTesting.java:302
>>> private <T extends MemberInfo> void addKey(boolean after, T memberInfo, Map<String, T> map) {
>>> if (after) {
>>> for (Iterator<Entry<String, T>> iterator = map.entrySet().iterator(); iterator.hasNext(); ) {
>>> Entry<String, T> info = iterator.next();
>>> if (info.getValue().equals(memberInfo)) {
>>> iterator.remove();
>>> break;
>>> }
>>> }
>>> map.put(memberInfo.toString(), memberInfo);
>>> }
>>> }
>>
>> Just do:
>>
>> map.entrySet().removeIf(e -> e.getValue().equals(memberInfo));
>>
>>
>> ReplToolTesting.java:571
>>> public Consumer<String> checkOutput() {
>>> String fullType = type.equals("@interface")? "annotation interface" : type;
>>> String expectedOutput = String.format("\\| *\\w+ %s %s", fullType, name);
>>> Pattern outputPattern = Pattern.compile(expectedOutput);
>>> return (s) -> {
>>> Matcher matcher = outputPattern.matcher(s);
>>> assertTrue(matcher.find(), "Expected: '" + expectedOutput + "', actual: " + s);
>>> };
>>> }
>>
>>
>> See Pattern.asPredicate, capture the returned predicate in the lambda instead of the pattern and call predicate.test.
>>
>>
>> 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.
>
> 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