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. > > 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. > > 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. > > 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. > > 4) There's a typo in the text for warn_init_list_type_narrowing. (OK, that > one's not a question...) > Since no test failed, this typo is conforming to our test suite. This is not an answer to your not a question.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
