[cfe-commits] [PATCH] Limit the number of overload candidates printed (issue1591041)

[Jeffrey Yasskin]

On Tue, Jun 8, 2010 at 2:02 PM, Jeffrey Yasskin <jyasskin at google.com> wrote:
> On Tue, Jun 8, 2010 at 2:01 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>
>> On Jun 8, 2010, at 1:59 PM, Jeffrey Yasskin wrote:
>>
>>> On Tue, Jun 8, 2010 at 1:52 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>>>
>>>> On Jun 8, 2010, at 1:06 PM, Jeffrey Yasskin wrote:
>>>>
>>>> On Tue, Jun 8, 2010 at 7:49 AM, Douglas Gregor <dgregor at apple.com> wrote:
>>>>>
>>>>> On Jun 7, 2010, at 6:03 PM, jyasskin at gmail.com wrote:
>>>>>
>>>>>> Reviewers: cfe-commits_cs.uiuc.edu,
>>>>>>
>>>>>> Message:
>>>>>> Please take a look. If you prefer reviewing diffs, they're behind the
>>>>>> "Download raw patch set" link.
>>>>>>
>>>>>> Description:
>>>>>> When there are lots of operator<<s, clang produces significantly worse
>>>>>> diagnostics than gcc, simply because of the size of the output. This
>>>>>> patch limits clang to 4 overload candidates, with the ability to show
>>>>>> the rest by passing -fshow-all-overloads, as a first cut. We'll want to
>>>>>> refine that later as examples of bad behavior come up.
>>>>>
>>>>>
>>>>> Unless we can be fairly sure that the "right" operator<< is in those first
>>>>> 4 overload candidates, I don't think this is a good idea. Unlike with
>>>>> suppressing inner template/macro instantiation histories, this change is
>>>>> likely to suppress important information.
>>>>>
>>>>
>>>> I agree that it will sometimes suppress important information. That's why I
>>>> added the -fshow-all-overloads flag so the user can get it back if they need
>>>> it.
>>>>
>>>> Sure, and it's good to have -fshow-all-overloads for any kind of pruning. My
>>>> concern is that if the pruning is not good by default, we'll end up causing
>>>> more harm than good: the user will have to bounce between
>>>> -fshow-all-overloads and non-fshow-all-overloads whenever they hit problems.
>>>> That's worse than having a longer diagnostic chain in the first place.
>>>>
>>>> But in cases like the one below, there are too many overloads printed to
>>>> find the "right" one, even if it were present, and they just discourage
>>>> users from reading any of them. 4 is clearly not the right cut-off in all
>>>> cases, and cutting off after a drop in quality is likely to be better in
>>>> many cases, but it fixes some of the most egregious behavior pretty easily.
>>>> We can fix places where it omits useful overloads as they come up.
>>>>
>>>> We can, but our heuristics are known not to be that good, so we won't even
>>>> have a good sense of how useful this change is until we have better
>>>> heuristics.
>>>>
>>>> If you prefer, I can look for a quality drop based
>>>> on CompareOverloadCandidatesForDisplay instead of the fixed cutoff. I'll
>>>> want a hard cutoff around 6-10 anyway, since at that point I think most
>>>> users give up on our errors and just stare at the source instead.
>>>>
>>>> I think a quality-based cutoff is the only workable solution, so IMO we need
>>>> that before we can turn this behavior on by default.
>>>> It would probably make sense to have the flag set
>>>> -fshow-overloads={best,all}
>>>> so that we have the option later of adding different tweaks/heuristics
>>>> (e.g., "detailed", to really show what happens for each overload).
>>>> Otherwise, we'll end up with several -fshow-*-overloads flags.
>>>
>>> That does sound better. Would you accept a -fshow-overloads={best,all}
>>> that defaulted to 'all' and had 'best' do the 4-overload cutoff, or
>>> would you want 'best' to look for a quality change in the first
>>> version?
>>
>> So long as the default remains "all" until we have decent heuristics for a quality-based cutoff, I'm happy to have support for "best" in the tree with the 4-overload cutoff.
>>
>
> Will do. Thanks!

This is done. New patch at
http://codereview.appspot.com/download/issue1591041_8001.diff