Most specific method in diagnostic generation for overload resolution

[Vicente Romero]

Hi Bernard,

Thanks for the patch. I was playing a bit with it and made some 
modifications to it, my changes are just in the put method of 
MostSpecificMap.

diff -r 19293ea3999f -r 102d38c4f033 
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
--- 
a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java 
Fri Sep 08 18:24:15 2017 +0000
+++ 
b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java 
Mon Sep 25 19:57:03 2017 -0400
@@ -59,6 +59,7 @@
  import java.util.HashSet;
  import java.util.Iterator;
  import java.util.LinkedHashMap;
+import java.util.LinkedList;
  import java.util.Map;
  import java.util.Set;
  import java.util.function.BiFunction;
@@ -3995,14 +3996,30 @@
          }
          //where
              private Map<Symbol, JCDiagnostic> mapCandidates() {
-                Map<Symbol, JCDiagnostic> candidates = new 
LinkedHashMap<>();
+                MostSpecificMap candidates = new MostSpecificMap();
                  for (Candidate c : resolveContext.candidates) {
                      if (c.isApplicable()) continue;
-                    candidates.put(c.sym, c.details);
+                    candidates.put(c);
                  }
                  return candidates;
              }

+            @SuppressWarnings("serial")
+            private class MostSpecificMap extends LinkedHashMap<Symbol, 
JCDiagnostic> {
+                private void put(Candidate c) {
+                    keySet().stream()
+                            .filter(s -> s != c.sym)
+                            .filter(s -> c.sym.overrides(s, 
(TypeSymbol)s.owner, types, false) )
+                            .forEach(s -> remove(s));
+
+                    if (!keySet().stream()
+                            .filter(s -> s == c.sym)
+                            .anyMatch(s -> s.overrides(c.sym, 
(TypeSymbol)c.sym.owner, types, false))) {
+                        put(c.sym, c.details);
+                    }
+                }
+            }
+
              Map<Symbol, JCDiagnostic> filterCandidates(Map<Symbol, 
JCDiagnostic> candidatesMap) {
                  Map<Symbol, JCDiagnostic> candidates = new 
LinkedHashMap<>();
                  for (Map.Entry<Symbol, JCDiagnostic> _entry : 
candidatesMap.entrySet()) {


in your previous version you were doing:

+                      new LinkedList<>(keySet()).stream()
+                            .filter( s -> !s.equals(c.sym) )
+                            .filter( s -> c.sym.overrides(s, (TypeSymbol)s.owner, types, false) )
+                            .forEach( s -> remove(s) );

by creating a brand new LinkedList my impression is that any removal of 
a key would happen in the linked list, not in the original map so the 
first statement was a no-op. Also as symbols are unique in javac you can 
safely use the "==" operator for comparison. Does my version captures 
what you intended?

In addition to all this I think that we should consider if List is the 
best data structure to store the candidates. It sounds like it could be 
a map to avoid duplicates, probably this won't be a great gain but there 
is no point on storing duplicates when we can check for them in O(1) time,

Thanks,
Vicente

On 09/20/2017 10:56 AM, B. Blaser wrote:
> Hi,
>
> Related to [1], I wrote a specialized map to store only the most
> specific candidates during the diagnostic generation for overload
> resolution, here under.
>
> It's then used in "Resolve.InapplicableSymbolsError.mapCandidates()"
> when non-applicable methods are collected to generate the diagnostic.
>
> What do you think?
> Bernard
>
> [1] http://mail.openjdk.java.net/pipermail/compiler-dev/2017-September/011062.html
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
> @@ -59,6 +59,7 @@
>   import java.util.HashSet;
>   import java.util.Iterator;
>   import java.util.LinkedHashMap;
> +import java.util.LinkedList;
>   import java.util.Map;
>   import java.util.Set;
>   import java.util.function.BiFunction;
> @@ -3995,14 +3996,29 @@
>           }
>           //where
>               private Map<Symbol, JCDiagnostic> mapCandidates() {
> -                Map<Symbol, JCDiagnostic> candidates = new LinkedHashMap<>();
> +                MostSpecificMap candidates = new MostSpecificMap();
>                   for (Candidate c : resolveContext.candidates) {
>                       if (c.isApplicable()) continue;
> -                    candidates.put(c.sym, c.details);
> +                    candidates.put(c);
>                   }
>                   return candidates;
>               }
>
> +            @SuppressWarnings("serial")
> +            private class MostSpecificMap extends
> LinkedHashMap<Symbol, JCDiagnostic> {
> +                private void put(Candidate c) {
> +                    new LinkedList<>(keySet()).stream()
> +                            .filter( s -> !s.equals(c.sym) )
> +                            .filter( s -> c.sym.overrides(s,
> (TypeSymbol)s.owner, types, false) )
> +                            .forEach( s -> remove(s) );
> +
> +                    if (!keySet().stream()
> +                            .filter( s -> !s.equals(c.sym) )
> +                            .anyMatch( s -> s.overrides(c.sym,
> (TypeSymbol)c.sym.owner, types, false) ))
> +                        put(c.sym, c.details);
> +                }
> +            }
> +
>               Map<Symbol, JCDiagnostic> filterCandidates(Map<Symbol,
> JCDiagnostic> candidatesMap) {
>                   Map<Symbol, JCDiagnostic> candidates = new LinkedHashMap<>();
>                   for (Map.Entry<Symbol, JCDiagnostic> _entry :
> candidatesMap.entrySet()) {

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20170925/4d51a7c7/attachment.html>