<div class="gmail_quote">On Thu, Jul 12, 2012 at 3:22 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>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></div><div><div>
<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></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></blockquote><div>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.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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>
</blockquote><div>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.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div>That is certainly possible to do, but also means that the diagnostic will be longer since the full types are printed.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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>
</blockquote><div>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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div>Since no test failed, this typo is conforming to our test suite. This is not an answer to your not a question.</div>
</div><br>