Most specific method in diagnostic generation for overload resolution

[Vicente Romero]

Hi Bernard,

On 09/26/2017 09:40 AM, B. Blaser wrote:
> 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.

yep I went to quick on this you are right :( we have to duplicate the 
keyset, I will apply your original patch with the change of !s.equals() 
for "!="

Thanks,
Vicente

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