<div>On Fri, Jul 13, 2012 at 3:41 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div class="im">On Thu, Jul 12, 2012 at 5:32 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 4:53 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">
<div class="gmail_quote"><div>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><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>
</div></blockquote><div><br></div></div><div>Sounds good to me.</div></div></blockquote></div><div>r<span style="color:rgb(34,34,34);font-size:13px;font-family:arial,sans-serif">160193</span> </div><div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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><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>
</div></blockquote><div><br></div></div><div>OK, this seems reasonable, and I suppose a diff for IdentityAliasTemplate<int> vs IdentityAliasTemplate<long> might be nice.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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><div>That is certainly possible to do, but also means that the diagnostic will be longer since the full types are printed.</div>
</div></blockquote><div><br></div></div><div>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.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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><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></div></blockquote><div><br></div></div>
<div>
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.</div>
</div>
</blockquote></div></div>Removed %diff from diagnostics where the types are expected to be different. Let me know if there should be more removed.
</blockquote></div><br><div>Thanks, LGTM!</div>