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

[Richard Trieu]

On Thu, Jul 12, 2012 at 3:22 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> 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?
>
The current diagnostics can't print two trees.  For the
err_init_conversion_failed, the first %diff prints if args 1 & 3 are
templates.  The other %diff's only print if 1 & 3 are function pointer
types.  Skimming the code, it appears that it does allow two trees to be
printed, but problems may arise if only one tree is requested.  I think
that printing the first possible tree, then ignoring any further trees
should be the way to go.

>
> 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?
>
The diff will almost always fall back to standard printing for some of the
diagnostics.  I applied the changes as broadly as possible in case anybody
else wanted to improve the %diff for other cases.

>
> 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>'
>
That is certainly possible to do, but also means that the diagnostic will
be longer since the full types are printed.

>
> 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.
>
Like I said, I applied the diff to as many places as I could.  I don't mind
changing the wording or removing %diff's if that makes things better.

>
> 4) There's a typo in the text for warn_init_list_type_narrowing. (OK, that
> one's not a question...)
>
Since no test failed, this typo is conforming to our test suite.  This is
not an answer to your not a question.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120712/d7d3086e/attachment.html>