REPL tool implementation code review

[Paul Sandoz]

On 9 Sep 2015, at 18:05, Robert Field <robert.field at oracle.com> wrote:
>> 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.
> 
> So, you are saying, for documentation purposes, I should declare it (now called argToSnippetSet) as returning LinkedHashSet, and the sets within the method (keySet now snippetSet and srcSet) as LinkedHashSet too? I thought the standard was to declare the interface and that one shouldn't declare them as their implementation type.
> 

When i looked at the cmdEdit method i could see it depends on an encounter order [*] of the Set returned from argToSnippetSet, but i don’t immediately know if the returned Set has an encounter order, it’s not defined in the method signature, i can look into the implementation and see it’s using LinkedHashSet and i know that state.snippets() returns a List so all is good, but what if some future developer inadvertently decides to change the argToSnippetSet implementation to use a HashSet instead of a LinkedHashSet?

It’s a subtle and smallish thing, but i think useful to capture the intent, as such encounter order bugs can be hard to track down.

Paul.

[*] i.e. guarantees concerning the order in which the elements are reported (or encountered) when the collection is traversed.