REPL tool implementation code review
Remi Forax
forax at univ-mlv.fr
Fri Sep 4 18:46:57 UTC 2015
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