RFR 8143964: JShell API: convert query responses to Stream instead of List/Collection

[ShinyaYoshida]

Hi Robert,
Thank you for your update.
I've confirmed. LGTM!

Regards,
shinyafox(Shinya Yoshida)

2016-08-10 10:26 GMT+09:00 Robert Field <robert.field at oracle.com>:

> Based on this review, I have used the first suggestion and changed the
> second to use streaming to filter, leaving a list which is almost always
> empty (this covers a rare case).  The loop is then cleaner.
>
>     // Check if there is a method whose user-declared parameter types are
>     // different (and thus has a different snippet) but whose compiled
> parameter
>     // types are the same. if so, consider it an overwrite replacement.
>     private Status overwriteMatchingMethod(MethodSnippet msi) {
>         String qpt = msi.qualifiedParameterTypes();
>         List<MethodSnippet> matching = state.methods()
>                 .filter(sn ->
>                            sn != null
>                         && sn != msi
>                         && sn.status().isActive()
>                         && sn.name().equals(msi.name())
>                         && qpt.equals(sn.qualifiedParameterTypes()))
>                 .collect(toList());
>
>         // Look through all methods for a method of the same name, with the
>         // same computed qualified parameter types
>         Status overwrittenStatus = null;
>         for (MethodSnippet sn : matching) {
>             overwrittenStatus = sn.status();
>             SnippetEvent se = new SnippetEvent(
>                     sn, overwrittenStatus, OVERWRITTEN,
>                     false, msi, null, null);
>             sn.setOverwritten();
>             secondaryEvents.add(se);
>             state.debug(DBG_EVNT,
>                     "Overwrite event #%d -- key: %s before: %s status: %s
> sig: %b cause: %s\n",
>                     secondaryEvents.size(), se.snippet(),
> se.previousStatus(),
>                     se.status(), se.isSignatureChange(),
> se.causeSnippet());
>         }
>         return overwrittenStatus;
>     }
>
> The only changes are these two loop changes (the spec is unchanged).  New
> webrev:
>
>      http://cr.openjdk.java.net/~rfield/8143964v1.webrev/
>
> Thanks,
> Robert
>
>
>
> On 08/09/16 17:45, Robert Field wrote:
>
>
> On 08/09/16 16:37, ShinyaYoshida wrote:
>
> Hi, Robert,
>
> I have 2 comments:
>
> 1895                 Stream<Snippet> sns = at.hasOption("-all")
> 1896                         ? state.snippets()
> 1897                         : state.snippets().filter(this::mainActive);
> 1898                 Iterator<Snippet> it = sns.iterator();
> 1899                 while (it.hasNext()) {
> 1900                     Snippet sn = it.next();
> 1901                     writer.write(sn.source());
> 1902                     writer.write("\n");
> 1903                 }
>
> You can use .collect method like following even if write throws IOE:
>
> String sources = (at.hasOption("-all")
>     ? state.snippets()
>     : state.snippets().filter(this::mainActive))
>     .map(Snippet::source)
>     .collect(Collectors.joining("\n"));
> writer.write(sources);
>
>
> I like that!
>
>
> In src/jdk.jshell/share/classes/jdk/jshell/Unit.java:
>          // Look through all methods for a method of the same name, with the
>          // same computed qualified parameter types
>          Status overwrittenStatus = null;-        for (MethodSnippet sn : state.methods()) {+        for (MethodSnippet sn : state.methods().collect(toList())) {
>              if (sn != null && sn != msi && sn.status().isActive() && sn.name().equals(msi.name())) {
>                  if (qpt.equals(sn.qualifiedParameterTypes())) {
>                      overwrittenStatus = sn.status();
>                      SnippetEvent se = new SnippetEvent(
>                              sn, overwrittenStatus, OVERWRITTEN,
>
> I'd like to avoid the collect(toList())+for-each because it need 2 pass for each data and kill the advantage of the streamification.
>
> Can you write this using StreamAPI or iterator from the stream?
>
>
> Unfortunately, this loop side-effects, it sets overwrittenStatus which is
> returned by the method.  Short of redesigning the set of methods that use
> this, which I don't want to do at this stage, directly using Stream won't
> work.
>
> I could switch to Iterator.  Since for-loops only take Iterable (not
> Iterator) it is clunky.  Though I did do it elsewhere (e.g. above).
>
> Note: this bug-fix removes a ".collect(toList()" which was always done
> behind the scenes.
>
> Thanks,
> Robert
>
> Regards,
>
> shinyafox(Shinya Yoshida)
>
>
> 2016-08-09 15:15 GMT+09:00 Robert Field <robert.field at oracle.com>:
>
>> Please review this deferred API review request.
>> Basically changing List<*> return to Stream<*> return for seven methods:
>>         Stream<Snippet> snippets()
>>         Stream<VarSnippet> variables()
>>         Stream<MethodSnippet> methods()
>>         Stream<TypeDeclSnippet> types()
>>         Stream<ImportSnippet> imports()
>>         Stream<Diag> diagnostics(Snippet snippet)
>>         Stream<String> unresolvedDependencies(DeclarationSnippet snippet)
>>
>> Bug:
>>     https://bugs.openjdk.java.net/browse/JDK-8143964
>>
>> Webrev:
>>     http://cr.openjdk.java.net/~rfield/8143964v0.webrev/
>>
>> Spec:
>> http://cr.openjdk.java.net/~rfield/8143964v0.jshellAPI/jdk/j
>> shell/JShell.html
>>
>> Specdiff:
>> http://cr.openjdk.java.net/~rfield/8143964v0.specdiff/jdk/js
>> hell/JShell.html
>>
>> Thanks,
>> Robert
>>
>>
>
>
>