[PATCH] Add an option to limit the number of overload candidates printed

When there are lots of operator<<s, clang produces significantly worse diagnostics than gcc, simply because of the size of the output. This patch adds a -fshow-diagnostics=best option to limit clang to 4 overload candidates, as a first cut.

[Jeffrey Yasskin, 2010-06-08 01:03:30]

Please take a look. If you prefer reviewing diffs, they're behind the "Download
raw patch set" link.

[dgregor_apple.com, 2010-06-08 14:49:34]

On Jun 7, 2010, at 6:03 PM, jyasskin@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.

	- Doug

[Jeffrey Yasskin (google), 2010-06-08 20:06:50]

On Tue, Jun 8, 2010 at 7:49 AM, Douglas Gregor <dgregor@apple.com> wrote:
>
> On Jun 7, 2010, at 6:03 PM, jyasskin@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. 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.

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'd rather not wait until the ordering is great before improving this at
all.


./util/gtl/stl_logging-inl.h:42:33: error: invalid operands to binary
expression ('basic_ostream<char, std::char_traits<char> >' and
'std::vector<int, std::allocator<int> > const')
 out << '(' << p.first << ", " << p.second << ')';
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~
In file included from ...:4:
In file included from ...:10:
In file included from ...:280:
In file included from ...:7:
In file included from ...:103:
In file included from ...:20:
In file included from ...:7:
In file included from ...:49:
In file included from ...:40:
In file included from ...:6:
.../include/c++/4.4.0/ostream:107:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'__ostream_type &(*)(__ostream_type &)' for 1st argument
     operator<<(__ostream_type& (*__pf)(__ostream_type&))
     ^
.../include/c++/4.4.0/ostream:116:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'__ios_type &(*)(__ios_type &)' for 1st argument
     operator<<(__ios_type& (*__pf)(__ios_type&))
     ^
.../include/c++/4.4.0/ostream:126:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'std::ios_base &(*)(std::ios_base &)' for 1st argument
     operator<<(ios_base& (*__pf) (ios_base&))
     ^
.../include/c++/4.4.0/ostream:164:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'long' for 1st argument
     operator<<(long __n)
     ^
.../include/c++/4.4.0/ostream:168:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'unsigned long' for 1st argument
     operator<<(unsigned long __n)
     ^
.../include/c++/4.4.0/ostream:172:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'bool' for 1st argument
     operator<<(bool __n)
     ^
.../include/c++/4.4.0/ostream:176:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'short' for 1st argument
     operator<<(short __n);
     ^
.../include/c++/4.4.0/ostream:179:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'unsigned short' for 1st argument
     operator<<(unsigned short __n)
     ^
.../include/c++/4.4.0/ostream:187:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'int' for 1st argument
     operator<<(int __n);
     ^
.../include/c++/4.4.0/ostream:190:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'unsigned int' for 1st argument
     operator<<(unsigned int __n)
     ^
.../include/c++/4.4.0/ostream:199:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'long long' for 1st argument
     operator<<(long long __n)
     ^
.../include/c++/4.4.0/ostream:203:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'unsigned long long' for 1st argument
     operator<<(unsigned long long __n)
     ^
.../include/c++/4.4.0/ostream:208:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'double' for 1st argument
     operator<<(double __f)
     ^
.../include/c++/4.4.0/ostream:212:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'float' for 1st argument
     operator<<(float __f)
     ^
.../include/c++/4.4.0/ostream:220:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'long double' for 1st argument
     operator<<(long double __f)
     ^
.../include/c++/4.4.0/ostream:224:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'void const *' for 1st argument
     operator<<(const void* __p)
     ^
.../include/c++/4.4.0/ostream:249:7: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'__streambuf_type *' (aka 'basic_streambuf<char, std::char_traits<char> >
*') for 1st argument
     operator<<(__streambuf_type* __sb);
     ^
.../include/c++/4.4.0/ostream:450:5: note: candidate function [with _CharT =
char, _Traits = std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'char' for 2nd argument
   operator<<(basic_ostream<_CharT, _Traits>& __out, char __c)
   ^
.../include/c++/4.4.0/ostream:456:5: note: candidate function [with _Traits
= std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'char' for 2nd argument
   operator<<(basic_ostream<char, _Traits>& __out, char __c)
   ^
.../include/c++/4.4.0/ostream:462:5: note: candidate function [with _Traits
= std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'signed char' for 2nd
argument
   operator<<(basic_ostream<char, _Traits>& __out, signed char __c)
   ^
.../include/c++/4.4.0/ostream:467:5: note: candidate function [with _Traits
= std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'unsigned char' for 2nd
argument
   operator<<(basic_ostream<char, _Traits>& __out, unsigned char __c)
   ^
.../include/c++/4.4.0/ostream:504:5: note: candidate function [with _Traits
= std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'char const *' for 2nd
argument
   operator<<(basic_ostream<char, _Traits>& __out, const char* __s)
   ^
.../include/c++/4.4.0/ostream:517:5: note: candidate function [with _Traits
= std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'signed char const *' for
2nd argument
   operator<<(basic_ostream<char, _Traits>& __out, const signed char* __s)
   ^
.../include/c++/4.4.0/ostream:522:5: note: candidate function [with _Traits
= std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'unsigned char const *'
for 2nd argument
   operator<<(basic_ostream<char, _Traits>& __out, const unsigned char* __s)
   ^
In file included from ...:4:
In file included from ...:10:
In file included from ...:280:
In file included from ...:7:
In file included from ...:103:
In file included from ...:20:
In file included from ...:7:
In file included from ...:49:
In file included from ...:40:
In file included from ...:6:
In file included from ...:564:
.../include/c++/4.4.0/bits/ostream.tcc:320:5: note: candidate function [with
_CharT = char, _Traits = std::char_traits<char>] not viable: no known
conversion from 'std::vector<int, std::allocator<int> > const' to 'char
const *' for 2nd argument
   operator<<(basic_ostream<_CharT, _Traits>& __out, const char* __s)
   ^
In file included from ...:4:
In file included from ...:10:
In file included from ...:280:
In file included from ...:7:
In file included from ...:103:
./base/logging.h:1205:10: note: candidate function not viable: no known
conversion from 'std::vector<int, std::allocator<int> > const' to
'PRIVATE_Counter const' for 2nd argument
ostream& operator<<(ostream &os, const PRIVATE_Counter&);
        ^
./base/logging.h:1212:17: note: candidate function not viable: no known
conversion from 'std::vector<int, std::allocator<int> > const' to 'DocId
const' for 2nd argument
inline ostream& operator<<(ostream& o, const DocId& d) {
               ^
./base/logging.h:1216:17: note: candidate function not viable: no known
conversion from 'std::vector<int, std::allocator<int> > const' to
'DocId32Bit const' for 2nd argument
inline ostream& operator<<(ostream& o, const DocId32Bit& d) {
               ^
./base/logging.h:1520:20: note: candidate function [with T =
std::vector<int, std::allocator<int> >] not viable: no known conversion from
'basic_ostream<char, std::char_traits<char> >' to 'NullStream &' for 1st
argument
inline NullStream& operator<<(NullStream &str, const T &value) { return str;
}
                  ^
In file included from ...:4:
In file included from ...:11:
In file included from ...:21:
In file included from ...:11:
In file included from ...:20:
In file included from ...:30:
In file included from ...:27:
In file included from ...:60:
./base/int128.h:70:17: note: candidate function not viable: no known
conversion from 'std::vector<int, std::allocator<int> > const' to 'uint128
const' for 2nd argument
extern ostream& operator<<(ostream& o, const uint128& b);
               ^
In file included from ...:4:
In file included from ...:11:
In file included from ...:21:
In file included from ...:11:
In file included from ...:20:
In file included from ...:30:
./strings/stringpiece.h:226:17: note: candidate function not viable: no
known conversion from 'std::vector<int, std::allocator<int> > const' to
'StringPiece const' for 2nd argument
extern ostream& operator<<(ostream& o, const StringPiece& piece);
               ^
In file included from ...:4:
In file included from ...:11:
In file included from ...:21:
In file included from ...:15:
In file included from ...:49:
In file included from ...:17:
In file included from ...:16:
In file included from ...:27:
./strings/cord.h:561:17: note: candidate function not viable: no known
conversion from 'std::vector<int, std::allocator<int> > const' to 'Cord
const' for 2nd argument
extern ostream& operator<<(ostream& out, const Cord& cord);
               ^
In file included from ...:17:
In file included from ...:14:
In file included from ...:13:
In file included from ...:111:
In file included from ...:125:
In file included from ...:6:
.../include/c++/4.4.0/iomanip:75:5: note: candidate function [with _CharT =
char, _Traits = std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'std::_Resetiosflags' for
2nd argument
   operator<<(basic_ostream<_CharT, _Traits>& __os, _Resetiosflags __f)
   ^
.../include/c++/4.4.0/iomanip:109:5: note: candidate function [with _CharT =
char, _Traits = std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'std::_Setiosflags' for
2nd argument
   operator<<(basic_ostream<_CharT, _Traits>& __os, _Setiosflags __f)
   ^
.../include/c++/4.4.0/iomanip:147:5: note: candidate function [with _CharT =
char, _Traits = std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'std::_Setbase' for 2nd
argument
   operator<<(basic_ostream<_CharT, _Traits>& __os, _Setbase __f)
   ^
.../include/c++/4.4.0/iomanip:220:5: note: candidate function [with _CharT =
char, _Traits = std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'std::_Setprecision' for
2nd argument
   operator<<(basic_ostream<_CharT, _Traits>& __os, _Setprecision __f)
   ^
.../include/c++/4.4.0/iomanip:254:5: note: candidate function [with _CharT =
char, _Traits = std::char_traits<char>] not viable: no known conversion from
'std::vector<int, std::allocator<int> > const' to 'std::_Setw' for 2nd
argument
   operator<<(basic_ostream<_CharT, _Traits>& __os, _Setw __f)
   ^
In file included from ...:4:
In file included from ...:6:
In file included from ...:6:
In file included from ...:52:
.../include/c++/4.4.0/bits/basic_string.h:2500:5: note: candidate template
ignored: failed template argument deduction
   operator<<(basic_ostream<_CharT, _Traits>& __os,
   ^
In file included from ...:4:
In file included from ...:6:
In file included from ...:48:
.../include/c++/4.4.0/ext/vstring.h:2407:5: note: candidate template
ignored: failed template argument deduction
   operator<<(basic_ostream<_CharT, _Traits>& __os,
   ^
In file included from ...:4:
In file included from ...:6:
third_party/stl/gcc3/string:550:3: note: candidate template ignored: failed
template argument deduction
 operator<<(std::basic_ostream<_CharT, _Traits>& __os,
 ^
In file included from ...:4:
In file included from ...:10:
In file included from ...:280:
In file included from ...:7:
In file included from ...:103:
In file included from ...:20:
In file included from ...:7:
In file included from ...:49:
In file included from ...:40:
In file included from ...:6:
.../include/c++/4.4.0/ostream:445:5: note: candidate template ignored:
deduced conflicting types for parameter '_CharT' ('char' vs.
'std::vector<int, std::allocator<int> >')
   operator<<(basic_ostream<_CharT, _Traits>& __out, _CharT __c)
   ^
.../include/c++/4.4.0/ostream:487:5: note: candidate template ignored:
failed template argument deduction
   operator<<(basic_ostream<_CharT, _Traits>& __out, const _CharT* __s)
   ^
In file included from ...:17:
In file included from ...:14:
In file included from ...:13:
In file included from ...:111:
In file included from ...:125:
In file included from ...:6:
.../include/c++/4.4.0/iomanip:186:5: note: candidate template ignored:
failed template argument deduction
   operator<<(basic_ostream<_CharT, _Traits>& __os, _Setfill<_CharT> __f)
   ^
In file included from ...:25:
./util/gtl/stl_logging-inl.h:41:17: note: candidate template ignored: failed
template argument deduction
inline ostream& operator<<(ostream& out, const pair<First, Second>& p) {
               ^
1 error generated.


NewCallback() (similar to tr1::bind) provides another wealth of spammy
overloads, with a mix of bad-arity and bad-conversion.

[dgregor_apple.com, 2010-06-08 20:52:40]

On Jun 8, 2010, at 1:06 PM, Jeffrey Yasskin wrote:

> On Tue, Jun 8, 2010 at 7:49 AM, Douglas Gregor <dgregor@apple.com> wrote:
> >
> > On Jun 7, 2010, at 6:03 PM, jyasskin@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.

	- Doug

[Jeffrey Yasskin (google), 2010-06-08 20:59:57]

On Tue, Jun 8, 2010 at 1:52 PM, Douglas Gregor <dgregor@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@apple.com> wrote:
>>
>> On Jun 7, 2010, at 6:03 PM, jyasskin@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?

[dgregor_apple.com, 2010-06-08 21:01:49]

On Jun 8, 2010, at 1:59 PM, Jeffrey Yasskin wrote:

> On Tue, Jun 8, 2010 at 1:52 PM, Douglas Gregor <dgregor@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@apple.com> wrote:
>>> 
>>> On Jun 7, 2010, at 6:03 PM, jyasskin@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.

	- Doug

[Jeffrey Yasskin (google), 2010-06-08 21:02:53]

On Tue, Jun 8, 2010 at 2:01 PM, Douglas Gregor <dgregor@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@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@apple.com> wrote:
>>>>
>>>> On Jun 7, 2010, at 6:03 PM, jyasskin@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!

[Jeffrey Yasskin (google), 2010-06-09 07:51:04]

On Tue, Jun 8, 2010 at 2:02 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> On Tue, Jun 8, 2010 at 2:01 PM, Douglas Gregor <dgregor@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@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@apple.com> wrote:
>>>>>
>>>>> On Jun 7, 2010, at 6:03 PM, jyasskin@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

[Jeffrey Yasskin (google), 2010-06-10 20:30:32]

Ping :)

On Wed, Jun 9, 2010 at 12:50 AM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> On Tue, Jun 8, 2010 at 2:02 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>> On Tue, Jun 8, 2010 at 2:01 PM, Douglas Gregor <dgregor@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@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@apple.com> wrote:
>>>>>>
>>>>>> On Jun 7, 2010, at 6:03 PM, jyasskin@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
>

[chandlerc, 2010-06-10 22:58:47]

Just a few tweaks I think, and looks good to me. Probably good for post-commit
review for anything else.

http://codereview.appspot.com/1591041/diff/8001/3006
File include/clang/Basic/Diagnostic.h (right):

http://codereview.appspot.com/1591041/diff/8001/3006#newcode179
include/clang/Basic/Diagnostic.h:179: 
Doc for the enum proper?

http://codereview.appspot.com/1591041/diff/8001/3006#newcode328
include/clang/Basic/Diagnostic.h:328: /// default, we omit candidates when
they're spammy.
Is this comment about the default still accurate? Also, do we want to specify
the default here, or with the flag's docs?

http://codereview.appspot.com/1591041/diff/8001/3004
File include/clang/Frontend/DiagnosticOptions.h (right):

http://codereview.appspot.com/1591041/diff/8001/3004#newcode39
include/clang/Frontend/DiagnosticOptions.h:39: /// Diagnostic::OverloadsShown
Is one bit always going to be enough then?

http://codereview.appspot.com/1591041/diff/8001/3012
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/1591041/diff/8001/3012#newcode5708
lib/Sema/SemaOverload.cpp:5708: "Condition when adding candidates should have
prevented this");
This shouldn't refer to the other code, but just the invariant imo: "Non-viable
builtin operators are not added to our reported set." or some such...

[Jeffrey Yasskin, 2010-06-11 05:58:20]

I've committed this as r105815. Let me know if anything needs to change.

http://codereview.appspot.com/1591041/diff/8001/3006
File include/clang/Basic/Diagnostic.h (right):

http://codereview.appspot.com/1591041/diff/8001/3006#newcode179
include/clang/Basic/Diagnostic.h:179: 
On 2010/06/10 22:58:47, chandlerc wrote:
> Doc for the enum proper?

Done.

http://codereview.appspot.com/1591041/diff/8001/3006#newcode328
include/clang/Basic/Diagnostic.h:328: /// default, we omit candidates when
they're spammy.
On 2010/06/10 22:58:47, chandlerc wrote:
> Is this comment about the default still accurate? Also, do we want to specify
> the default here, or with the flag's docs?

Oops, updated.
It seems to need to be in multiple places. Diagnostics has one default when you
fail to set it. DiagnosticOptions has another. The flag inherits the default
from DiagnosticOptions.

http://codereview.appspot.com/1591041/diff/8001/3004
File include/clang/Frontend/DiagnosticOptions.h (right):

http://codereview.appspot.com/1591041/diff/8001/3004#newcode39
include/clang/Frontend/DiagnosticOptions.h:39: /// Diagnostic::OverloadsShown
On 2010/06/10 22:58:47, chandlerc wrote:
> Is one bit always going to be enough then?

Who knows. We'll have to remember to expand it if we need more bits. Is that a
problem?

http://codereview.appspot.com/1591041/diff/8001/3012
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/1591041/diff/8001/3012#newcode5708
lib/Sema/SemaOverload.cpp:5708: "Condition when adding candidates should have
prevented this");
On 2010/06/10 22:58:47, chandlerc wrote:
> This shouldn't refer to the other code, but just the invariant imo:
"Non-viable
> builtin operators are not added to our reported set." or some such...

Done.

[clattner_apple.com, 2010-06-13 05:23:28]

On Jun 8, 2010, at 1:52 PM, Douglas Gregor wrote:

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

I strongly agree with Doug here.  IMO, requiring people to go stare at their
code *is ok*, but requiring them to go twiddle a command line option is gross
(realistically it's a non-starter for GUI users for example).

I think there are at least three important cases here:

1. The candidate list is short enough that all can be listed (e.g. <= 8), just
list them.

2. The candidate list is long, but some of the matches are much better than most
of them.  In this case, I'd like to see something like this:

error: no match for 'foo'
note: candidate 1 (best one)
note: candidate ...
note: candidate 6 (a close one but not right)
note: 42 candidates elided because they were way too wrong

When getting this sort of diagnostic, I'm okay with forcing the user to go stare
at their code again.  With 48 candidates, I agree that listing them is useless.

3. The candidate list is long and there is no "particularly close" match.  This
is the operator<< case with ostream when you forget to include the header that
defines the right << for example.  Here there is very little point in listing
*any* of the candidates.  I think that this should go into a super-concise mode
that could even list multiple candidates per line or something with no location
information.

It may also be useful to distinguish overloading cases where all the candidates
are members a class vs they include free functions.  In any case, I am really
opposed to a hard truncation at a fixed candidate limit.

-Chris 

[dgregor_apple.com, 2010-06-14 14:33:16]

On Jun 9, 2010, at 12:50 AM, Jeffrey Yasskin wrote:

> On Tue, Jun 8, 2010 at 2:02 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>> On Tue, Jun 8, 2010 at 2:01 PM, Douglas Gregor <dgregor@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@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@apple.com> wrote:
>>>>>> 
>>>>>> On Jun 7, 2010, at 6:03 PM, jyasskin@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