<div class="gmail_quote">On Thu, Jul 12, 2012 at 2:07 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<div>

<br></div><div>Patch attached and also available at:</div><div><a href="http://llvm-reviews.chandlerc.com/D6" target="_blank">http://llvm-reviews.chandlerc.com/D6</a></div></blockquote></div><div><br></div><div>This looks awesome! I have a few questions:</div>
<div><br></div><div>1) A few of the diagnostics contain multiple %diff{...}s, for instance, err_init_conversion_failed. How well does that work in tree mode?</div><div><br></div><div>2) How should we handle diagnostics which refer only to built-in types? (Examples: the warn_impcast_* and most of the err_typecheck_* diagnostics.)</div>
<div><br></div><div>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?</div>
<div><br></div><div>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:</div>
<div><div><br></div><div>'implicit conversion changes value from %2 to %3; types are:</div><div><tree>'</div></div><div><br></div><div>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.)</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>4) There's a typo in the text for warn_init_list_type_narrowing. (OK, that one's not a question...)</div>