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

[Douglas Gregor]

On Jun 9, 2010, at 12:50 AM, Jeffrey Yasskin 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


Looks good.

	- Doug