REPL tool implementation code review

[Paul Sandoz]

Hi Robert,

The changesets look good.


On 9 Sep 2015, at 03:32, Robert Field <robert.field at oracle.com> wrote:
>> 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.
> 

But the argToKeySet() method returns a Set, which may or may not have a defined encounter order, it’s not clear and would be more so if argToKeySet returned LinkedHashSet.


>> 
>> 
>> 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());
>        }
>    }
> 

Ok. I cannot recall if there was or you added a comment regarding normalization and termination, if not it might be useful to do so, in case someone comes along later on and inadvertently changes it.

Paul.