REPL tool implementation code review
Robert Field
robert.field at oracle.com
Wed Sep 9 01:32:33 UTC 2015
On 09/04/15 10: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.
Thanks again, Paul for the detailed review. I've responded inline or
fixed the issue for all the items in my areas except the first two.
>
> 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?
Yes. Working on this.
>
>
> 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)
See separate email.
>
> 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.
Should be default charset.
>
>
> jdk/internal/jshell/tool/JShellTool.java:582
>> int lastSlash = code.lastIndexOf('/'); //XXX
> Rogue comment.
Jan fixed.
>
>
> 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))
Jan has updated.
>
>
>> 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.
Jan has updated.
>
>
> 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.
Half the commands take no args, consistency might be more important than
using method reference (in half the cases).
>
>
> 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.
Jan has updated.
>
>
> 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.
It cannot be, currently, be null. This was defensive programming. But
it was applied inconsistently. Removed.
>
>
> 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.
Used, also fixed naming.
>
>
>> 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));
>
Well, I did change to using a method reference and fixed naming.
> 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?
It does use 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.
Done, used explicit list of StandardOpenOption to make write options
explicit and did related clean-up using Path instead of String.
>
>
> 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());
Cool! Done.
>
>
> jdk/internal/jshell/tool/JShellTool.java:1710
>> public ScannerIOContext(InputStream inStream, PrintStream pStream) {
>> this(new Scanner(inStream), pStream);
>> }
> Unused constructor.
I'll leave for Jan.
>
>
> 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?
I want the normalization and termination. Used Remi's suggestion,
making it:
private void saveFile() {
try {
saveHandler.accept(Files.lines(tmpfile).collect(Collectors.joining("\n",
"", "\n")));
} catch (IOException ex) {
errorHandler.accept("Failure in read edit file: " +
ex.getMessage());
}
}
>
>> 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?
Exactly!
>
>
> 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.
I'd like to leave it for possible future use and symmetry with
ExternalEditor.
>
> Is this still experimental? Is so should be it included?
I think it is a useful default., removed "Experimental"
>
>
> 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.
Clean! I rewrote as this (not sure about interrupt handling):
public static void edit(Consumer<String> errorHandler, String
initialText,
Consumer<String> saveHandler) {
CountDownLatch closeLock = new CountDownLatch(1);
SwingUtilities.invokeLater(
new EditPad(errorHandler, initialText, closeLock,
saveHandler));
do {
try {
closeLock.await();
break;
} catch (InterruptedException ex) {
// ignore and loop
}
} while (true);
}
The rest are Jan's and Andrei's...
-Robert
More information about the kulla-dev
mailing list