Print additional information when two function pointers are printed in a diagnostic.

Committed.

[Richard Smith, 2011-11-15 22:35:36]

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1018: "%select{| because of different
number of parameters (expected %5 but has %6)"
Having experienced the ambiguity of GHC's 'expected versus inferred'
diagnostics, I think I'd prefer to see just (%5 vs %6), where the correspondence
can be inferred from the ordering.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1018: "%select{| because of different
number of parameters (expected %5 but has %6)"
Would the wording be improved by using just ": " instead of " because of "?

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1019: "| type mismatch at %ordinal5
parameter (expected %6 but has %7)"
Should this be "| because of type mismatch [...]"?

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1019: "| type mismatch at %ordinal5
parameter (expected %6 but has %7)"
I think 'in %ordinal5 parameter' would read more naturally than 'at %ordinal5
parameter'.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1027: "%select{s|||s||s|s|s}6)}4">;
It seems redundant to include the word 'qualifier' three times in this
diagnostic. Would it be clear enough to say: "different qualifiers (none vs
'const volatile')"?

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1837: "%select{| has different number
of parameters (expected %3 but has %4)"
expected/has works well here, since it's clear which one is which.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1840: "| because of different
qualifiers (expected "
Should this be "| has different qualifiers [...]"?

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:4050: "%select{| has different number
of parameters (expected %5 but has %6)"
Did you intend for there to be two spaces between the ';' and the 'has' in these
diagnostics? Also, it might be clearer to use a ':' rather than a ';' for the
new message parts, and drop the 'has'. I think it'd be clear enough to use:
  error: assigning to 'int (*)()' from incompatible type 'char (*)()': different
return types ('int' vs 'char')

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:4053: "| because of different
qualifiers (expected "
If you keep the 'has', this should be "| has different qualifiers".

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2185: ToType = ToType->getPointeeType();
If you're happy with a tiny bit of scope creep, it'd be nice to also strip off
reference types.

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2233: << FromFunction->getResultType();
error: missing 'return;' before '}'

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2262: return false;
under-indented statement

[rtrieu, 2011-11-17 00:07:28]

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1018: "%select{| because of different
number of parameters (expected %5 but has %6)"
On 2011/11/15 22:35:36, Richard Smith wrote:
> Having experienced the ambiguity of GHC's 'expected versus inferred'
> diagnostics, I think I'd prefer to see just (%5 vs %6), where the
correspondence
> can be inferred from the ordering.

Done.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1018: "%select{| because of different
number of parameters (expected %5 but has %6)"
On 2011/11/15 22:35:36, Richard Smith wrote:
> Would the wording be improved by using just ": " instead of " because of "?

No idea, but it's not a big deal.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1019: "| type mismatch at %ordinal5
parameter (expected %6 but has %7)"
On 2011/11/15 22:35:36, Richard Smith wrote:
> Should this be "| because of type mismatch [...]"?

Done.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1019: "| type mismatch at %ordinal5
parameter (expected %6 but has %7)"
On 2011/11/15 22:35:36, Richard Smith wrote:
> I think 'in %ordinal5 parameter' would read more naturally than 'at %ordinal5
> parameter'.

Done.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1027: "%select{s|||s||s|s|s}6)}4">;
On 2011/11/15 22:35:36, Richard Smith wrote:
> It seems redundant to include the word 'qualifier' three times in this
> diagnostic. Would it be clear enough to say: "different qualifiers (none vs
> 'const volatile')"?

Done.  Removed here and below.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1840: "| because of different
qualifiers (expected "
On 2011/11/15 22:35:36, Richard Smith wrote:
> Should this be "| has different qualifiers [...]"?

Done.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:4050: "%select{| has different number
of parameters (expected %5 but has %6)"
On 2011/11/15 22:35:36, Richard Smith wrote:
> Did you intend for there to be two spaces between the ';' and the 'has' in
these
> diagnostics? Also, it might be clearer to use a ':' rather than a ';' for the
> new message parts, and drop the 'has'. I think it'd be clear enough to use:
>   error: assigning to 'int (*)()' from incompatible type 'char (*)()':
different
> return types ('int' vs 'char')

The ";" came from the previous fixit hint.  I merged the semi colon into its
select text.

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:4053: "| because of different
qualifiers (expected "
On 2011/11/15 22:35:36, Richard Smith wrote:
> If you keep the 'has', this should be "| has different qualifiers".

Changed to ": " format.

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2185: ToType = ToType->getPointeeType();
On 2011/11/15 22:35:36, Richard Smith wrote:
> If you're happy with a tiny bit of scope creep, it'd be nice to also strip off
> reference types.

Done.

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2233: << FromFunction->getResultType();
On 2011/11/15 22:35:36, Richard Smith wrote:
> error: missing 'return;' before '}'

Done.

http://codereview.appspot.com/5369119/diff/2001/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2262: return false;
On 2011/11/15 22:35:36, Richard Smith wrote:
> under-indented statement

Done.

[Richard Smith, 2011-11-17 22:41:37]

http://codereview.appspot.com/5369119/diff/6002/include/clang/Basic/Diagnosti...
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/5369119/diff/6002/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1838: "| has type mismatch at
%ordinal3 parameter (expected %4 but has %5)"
"in" rather than "at" for consistency, please.

http://codereview.appspot.com/5369119/diff/6002/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:4048: "%select{|: different classes
(expected %5 but has %6)"
I'd prefer "vs" rather than "expected/has" for this one too.

http://codereview.appspot.com/5369119/diff/6002/lib/Sema/SemaOverload.cpp
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/5369119/diff/6002/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2186: << QualType(ToMember->getClass(), 0);
The types here seem to be in the wrong order.

http://codereview.appspot.com/5369119/diff/6002/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2200: ToType = ToType.getNonReferenceType();
It might be worth using a separate mismatch message for the case where the
references are of different kinds (lvalue vs rvalue references). Also a test for
this case would be great.

[rtrieu, 2011-11-23 02:59:42]

http://codereview.appspot.com/5369119/diff/6002/include/clang/Basic/Diagnosti...
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/5369119/diff/6002/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:1838: "| has type mismatch at
%ordinal3 parameter (expected %4 but has %5)"
On 2011/11/17 22:41:37, Richard Smith wrote:
> "in" rather than "at" for consistency, please.

Done.

http://codereview.appspot.com/5369119/diff/6002/include/clang/Basic/Diagnosti...
include/clang/Basic/DiagnosticSemaKinds.td:4048: "%select{|: different classes
(expected %5 but has %6)"
On 2011/11/17 22:41:37, Richard Smith wrote:
> I'd prefer "vs" rather than "expected/has" for this one too.

Done.

http://codereview.appspot.com/5369119/diff/6002/lib/Sema/SemaOverload.cpp
File lib/Sema/SemaOverload.cpp (right):

http://codereview.appspot.com/5369119/diff/6002/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2186: << QualType(ToMember->getClass(), 0);
On 2011/11/17 22:41:37, Richard Smith wrote:
> The types here seem to be in the wrong order.

Done.

http://codereview.appspot.com/5369119/diff/6002/lib/Sema/SemaOverload.cpp#new...
lib/Sema/SemaOverload.cpp:2200: ToType = ToType.getNonReferenceType();
On 2011/11/17 22:41:37, Richard Smith wrote:
> It might be worth using a separate mismatch message for the case where the
> references are of different kinds (lvalue vs rvalue references). Also a test
for
> this case would be great.

After a chat with Richard Smith, it was determined that this case is not
applicable to the type of code this diagnostic runs into.