[cfe-commits] Print additional information when two function pointers are printed in a diagnostic. (issue 5369119)

rtrieu at google.com rtrieu at google.com
Wed Nov 16 16:07:29 PST 2011


Reviewers: Richard Smith,


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

http://codereview.appspot.com/5369119/diff/2001/include/clang/Basic/DiagnosticSemaKinds.td#newcode1018
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/DiagnosticSemaKinds.td#newcode1018
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/DiagnosticSemaKinds.td#newcode1019
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/DiagnosticSemaKinds.td#newcode1019
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/DiagnosticSemaKinds.td#newcode1027
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/DiagnosticSemaKinds.td#newcode1840
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/DiagnosticSemaKinds.td#newcode4050
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/DiagnosticSemaKinds.td#newcode4053
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#newcode2185
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#newcode2233
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#newcode2262
lib/Sema/SemaOverload.cpp:2262: return false;
On 2011/11/15 22:35:36, Richard Smith wrote:
> under-indented statement

Done.



Please review this at http://codereview.appspot.com/5369119/

Affected files:
   M     include/clang/Basic/DiagnosticSemaKinds.td
   M     include/clang/Sema/Sema.h
   M     lib/Sema/SemaExpr.cpp
   M     lib/Sema/SemaInit.cpp
   M     lib/Sema/SemaOverload.cpp
   M     lib/Sema/SemaTemplateDeduction.cpp
   M     test/SemaCXX/addr-of-overloaded-function.cpp





More information about the cfe-commits mailing list