REPL tool implementation code review

[Paul Sandoz]

Hi,

Some comments on Remi’s comments, don’t worry the set keeps getting smaller :-)


On 4 Sep 2015, at 20:46, Remi Forax <forax at univ-mlv.fr> wrote:
>>> 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(…)

Yes, good point.


>> 
>> 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().
> 

Yes, i missed the transition key.source().


>> 
>> 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”))
> 

I thought that initially too, but then IIRC the list of lines is consumed to reproduce the file as a string, AFAICT the only difference being is a CR/LF is guaranteed at the end.


>> 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.
> 

Yes, try to avoid using stream.iterator() unless absolutely necessary.

Paul.