On 05/18/2011 01:45 PM, Jason Merrill wrote:
>> Thanks for the background; I will keep the principle in mind. IMHO, in
>> a case like this where we're logically printing one diagnostic (one
>> error and then some number of explanatory notes) keeping all the logic
>> for the diagnostic centralized makes more sense.
>
> I understand, but that means we have to create a whole data structure to try
> and preserve information about the failure, and either having to duplicate
> every possible error or give less informative messages. I feel even more
> strongly about this after looking more closely at your patch.
Thank you for the review. I'll go back and try things the way you suggest;
before I go off and do that, I've taken your comments to mean that:
- fn_type_unification/type_unification_real and associated callers should take
a boolean `explain' parameter, which is normally false;
- failed calls to fn_type_unification should save the arguments for the call
for future explanation;
- printing diagnostic messages should call fn_type_unification with the saved
arguments and a true `explain' parameter.
This is similar to passing `struct unification_info' and really only involves
shuffling code from call.c into the unify_* functions in pt.c and some minor
changes to the rejection_reason code in call.c. The only wrinkle I see is
that in cases like these:
>> if (TREE_PURPOSE (TREE_VEC_ELT (tparms, i)))
>> {
>> tree parm = TREE_VALUE (TREE_VEC_ELT (tparms, i));
>> tree arg = TREE_PURPOSE (TREE_VEC_ELT (tparms, i));
>> arg = tsubst_template_arg (arg, targs, tf_none, NULL_TREE);
>> arg = convert_template_argument (parm, arg, targs, tf_none,
>> i, NULL_TREE, ui);
>> if (arg == error_mark_node)
>> return unify_parameter_deduction_failure (ui, parm);
>
> In this case, the problem is that we tried to use the default template
> argument but it didn't work for some reason; we should say that, not just say
> we didn't deduce something, or the users will say "but there's a default
> argument!".
>
> In this case, we should do the substitution again with tf_warning_or_error so
> the user can see what the problem actually is, not just say that there was
> some unspecified problem.
>
>> if (coerce_template_parms (parm_parms,
>> full_argvec,
>> TYPE_TI_TEMPLATE (parm),
>> tf_none,
>> /*require_all_args=*/true,
>> /*use_default_args=*/false, ui)
>> == error_mark_node)
>> return 1;
>
> Rather than pass ui down into coerce_template_parms we should just note when
> it fails and run it again at diagnostic time.
>
>> converted_args
>> = (coerce_template_parms (tparms, explicit_targs, NULL_TREE, tf_none,
>> /*require_all_args=*/false,
>> /*use_default_args=*/false, ui));
>> if (converted_args == error_mark_node)
>> return 1;
>
> Here too.
>
>> if (fntype == error_mark_node)
>> return unify_substitution_failure (ui);
>
> And this should remember the arguments so we can do the tsubst again at
> diagnostic time.
and other bits of pt.c, I'm interpreting your suggestions to mean that
tf_warning_or_error should be passed if `explain' is true. That doesn't seem
like the best interface for diagnostics, as we'll get:
foo.cc:105:40 error: no matching function for call to bar (...)
foo.cc:105:40 note: candidates are:
bar.hh:7000:30 note: bar (...)
bar.hh:7000:30 note: [some reason]
bar.hh:4095:63 note: bar (...)
bar.hh:....... error: [some message from tf_warning_or_error code]
I'm not sure that the last location there will necessary be the same as the
one that's printed for the declaration. I think I'll punt on that issue for
the time being until we see how the diagnostics work out. There's also the
matter of the error vs. note diagnostic. I think it'd be nicer to keep the
conformity of a note for all the explanations; the only way I see to do that
is something like:
- Add a tf_note flag; pass it at all appropriate call sites when explaining
things;
- Add a tf_issue_diagnostic flag that's the union of tf_{warning,error,note};
- Change code that looks like:
if (complain & tf_warning_or_error)
error (<STUFF>);
to something like:
if (complain & tf_issue_diagnostic)
emit_diagnostic (complain & tf_note ? DK_NOTE : DK_ERROR, <STUFF>);
passing input_location if we're not already passing a location.
That involves a lot of code churn. (Not a lot if you just modified the
functions above, but with this scheme, you'd have to call instantiate_template
again from the diagnostic code, and I assume you'd want to call that with
tf_note as well, which means hitting a lot more code.) I don't see a better
way to keep the diagnostics uniform, but I might be making things too
complicated; did you have a different idea of how to implement what you were
suggesting?
-Nathan
