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

[Richard Trieu]

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

> On Thu, Jul 12, 2012 at 4:53 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> 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.
>>
>
> Sounds good to me.
>
r160193

>
>
>> 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.
>>
>
> OK, this seems reasonable, and I suppose a diff for
> IdentityAliasTemplate<int> vs IdentityAliasTemplate<long> might be nice.
>
>
>> 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.
>>
>
> In the above example, %2 and %3 there aren't the types in question, and
> I'm only suggesting changing the tree form of the diagnostic. If my
> counting is right, the tweak should make that diagnostic 1 character
> shorter.
>
>
>> 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.
>>
>
> I think it would be useful to have a general policy for this. For that, I
> suggest: "%diff should be used when a diagnostic contains two types which
> the user might reasonably have expected to be the same. It should not be
> used when a diagnostic contains two types which were likely to have been
> intentionally different." -- unless we have a motivating example for
> wanting a diff in the latter case.
>
Removed %diff from diagnostics where the types are expected to be
different.  Let me know if there should be more removed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120713/8f4a4a41/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: diagnostics-diff-update2.patch
Type: application/octet-stream
Size: 36350 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120713/8f4a4a41/attachment.obj>