On Fri, Jul 13, 2012 at 3:41 PM, Richard Trieu <[email protected]> wrote:
> On Thu, Jul 12, 2012 at 5:32 PM, Richard Smith <[email protected]>wrote: > >> On Thu, Jul 12, 2012 at 4:53 PM, Richard Trieu <[email protected]> wrote: >> >>> On Thu, Jul 12, 2012 at 3:22 PM, Richard Smith <[email protected]>wrote: >>> >>>> On Thu, Jul 12, 2012 at 2:07 PM, Richard Trieu <[email protected]>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. Thanks, LGTM!
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
