Most specific method in diagnostic generation for overload resolution

[B. Blaser]

Hi Vicente,

Thanks for looking at this.

On 26 September 2017 at 02:14, Vicente Romero <vicente.romero at oracle.com> wrote:
> 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.
>
> 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.

"s -> remove(s)" is a closure on "MostSpecificMap.put()".
So, "remove(s)" is applied on the map (if all goes well), which is
what we expect, I think.

I thought that duplicating the *keyset* was necessary because we can't
modify the map ("remove(s)") while iterating on it, but we have to
verify this.

> Also as symbols are unique in javac you can safely
> use the "==" operator for comparison.

Thanks for the confirmation (I wasn't enough sure to safely use "!=") !
Note that this is "!=", not "==" as you used in:

+                    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);

Here, we would like to add a candidate if no method overrides it,
*except* itself.

> Does my version captures what you intended?

We just need to be sure that removing elements in the map is possible
while iterating on it, as explained above.

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

I've already tried not to duplicate candidates but "mapCandidates()"
was already using a "LinkedHashMap" and the rest of the diagnostic
generation is built above that.

Notice also that while "c.sym" could be added twice, the corresponding
"c.details" would be different.

So, using another structure would probably be more complicated I
think. And while I'm very concerned about performances, I think this
wouldn't be a great gain here.

Do you agree?

Thanks,
Bernard

> Thanks,
> Vicente