[cfe-commits] [Patch] Update Sema Diagnostics to use %diff

[Richard Smith]

On Thu, Jul 12, 2012 at 2:07 PM, Richard Trieu <rtrieu at google.com> wrote:

> Change diagnostic messages in DiagnosticsSemaKinds.td to use %diff when
> two QualType's are required.  This will enable template type diffing for
> all Sema messages.  The default text of the diff matches the old message so
> no test cases need to be updated.  Some tests have been added for the
> modified messages.
>
> Patch attached and also available at:
> http://llvm-reviews.chandlerc.com/D6
>

This looks awesome! I have a few questions:

1) A few of the diagnostics contain multiple %diff{...}s, for instance,
err_init_conversion_failed. How well does that work in tree mode?

2) How should we handle diagnostics which refer only to built-in types?
(Examples: the warn_impcast_* and most of the err_typecheck_* diagnostics.)

Is the diff valuable here? Are there cases where this will cause worse
diagnostics? More generally, how hesitant should we be before adding %diff
to a diagnostic, and are there any rules we should be following?

I'm not overly fond of the "between types" wording in the tree case for the
warn_impcast_* set (it seems redundant to say that a cast is between types,
although I see that you need some way to introduce the type tree). I wonder
if something like this would read more naturally:

'implicit conversion changes value from %2 to %3; types are:
<tree>'

3) How should we handle diagnostics are just naming two types which weren't
expected to be the same, rather than comparing them to each other?
(Examples: most of the cases where the diagnostic concerns an explicit
cast.)

In such a case, I would expect the way that the types differ would be
uninteresting. My concern in such cases is mainly that adding a diff might
make the diagnostic less clear, in the case where the diff either performs
some desugaring or produces a tree.

This also applies to err_ovl_ambiguous_oper_binary
and err_typecheck_invalid_operands. Those are a little more interesting,
because I suppose it will frequently be the case that the operands to a
binary operator were supposed to be the same. However, I think it could be
confusing to produce a type tree for these, since the diagnostic is
fundamentally trying to list two types, not point out the difference
between two types.

4) There's a typo in the text for warn_init_list_type_narrowing. (OK, that
one's not a question...)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120712/f3e915ae/attachment.html>