REPL tool implementation code review
Andrei Eremeev
andrei.eremeev at oracle.com
Thu Sep 10 12:27:41 UTC 2015
Thank for your comments, Paul!
The test code was fixed.
Andrei
On 04.09.2015 20:19, 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?
>
>
> 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.
>
>
> 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.
>
>
> 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"))
>
>
> 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.
>
>
> 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());
>
>
> 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?
>
>> 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.
>
>
> 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.
>
More information about the kulla-dev
mailing list