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

[Jeffrey Yasskin]

Ping :)

On Wed, Jun 9, 2010 at 12:50 AM, Jeffrey Yasskin <jyasskin at google.com> wrote:
> 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
>