On Mon, 2017-05-08 at 07:36 -0400, Nathan Sidwell wrote: > On 05/04/2017 07:44 PM, David Malcolm wrote: > > This patch kit implements two new options to make it easier > > to read diagnostics involving mismatched template types: > > -fdiagnostics-show-template-tree and > > -fno-elide-type. > > > > It adds two new formatting codes: %H and %I which are > > equivalent to %qT, but are to be used together for type > > comparisons e.g. > > "can't convert from %H to %I". > > While I can see why one might want %H and %I to imply quoting, it > seems > confusingly inconsistent with the other formatters, which require an > explicit 'q'.
This an implementation detail leaking through, but I *think* it's fixable. The implementation has to jump through some hoops to interact with pp_format, because the two codes interact with each other: in order to elide commonality and colorize differences we can't start printing the %H until we've seen the %I (vice versa should work also). The 'q' for quoting the next format code happens in the 2nd phase of pp_format: if 'q' is present, then a quote (and potentially colorization) is emitted to the chunk for the item before and after the item's main content is emitted to the chunk. To make the interaction of %H and %I work, we have to delay the writing to the chunk to a new phase between phases 2 and 3 (and stash a pointer to the chunk we're going to write to). As written, if one uses '%qH' and '%qI', then phase 2 writes the open and close quotes to the chunks, and then the delayed handler blows that away and overwrites the chunk content with (forcibly) quoted content for the types. So I think it can work if we add a "needs quoting" flag to the postprocessing phase, if we need to handle the case where %H and %I ever appear without 'q' (and have the delayed handling stash that flag, and do the quoting there). I'll look at implementing that. > > rather than printing: > > > > could not convert 'std::map<int, std::vector<double> >()' > > from 'std::map<int, std::vector<double> >' to 'std::map<int, > > std::vector<float> >' > > > > with -felide-type (the default), it prints: > > > > could not convert 'std::map<int, std::vector<double> >()' > > from 'map<[...],vector<double>>' to 'map<[...],vector<float>> > > Neat. > > Is '-felide-type' a good name? Wouldn't something like > '-fdiagnostic-elide-template-args' be better? Here I'm merely copying clang's option name. http://clang.llvm.org/docs/UsersManual.html#cmdoption-fno-elide-type > > With -fdiagnostics-show-template-tree a tree-like structure of the > > template is printed, showing the differences; in this case: > > 'tree' sounds implementor-speaky, Perhaps > '-fdiagnostic-expand-template-args' or something? As Jason noted, this is a user-visible thing that's tree-like, rather than our "tree" type. Also, I'm (shamelessly) copying clang's name for this. > Right, bikeshedding out of the way ... > > > --- a/gcc/cp/error.c > > +++ b/gcc/cp/error.c > > @@ -99,7 +99,47 @@ static void cp_diagnostic_starter > > (diagnostic_context *, diagnostic_info *); > > static void cp_print_error_function (diagnostic_context *, > > diagnostic_info *); > > > > > + > > +/* Return true iff TYPE_A and TYPE_B are template types that are > > + meaningful to compare. */ > > + > > +static bool > > +comparable_template_types_p (tree type_a, tree type_b) > > +{ > > + if (TREE_CODE (type_a) != RECORD_TYPE) > > + return false; > > + if (TREE_CODE (type_b) != RECORD_TYPE) > > + return false; > > CLASS_TYPE_P (type_a) etc? I'll use this in the next version. > > + int len_max = MAX (len_a, len_b); > > + for (int idx = 0; idx < len_max; idx++) > > + { > > + tree arg_a = idx < len_a ? TREE_VEC_ELT (args_a, idx) : > > NULL; > > + tree arg_b = idx < len_b ? TREE_VEC_ELT (args_b, idx) : > > NULL; > > + if (arg_a == arg_b) > > Should this use same_type_p for types? If direct comparison is > correct, > a comment would help. What about non-type template arguments? That said, I just tried one, and it's handled poorly by my patch. I'll try to address for the next version. > > + Only non-default template arguments are printed. */ > > + > > +static void > > +print_template_tree_comparison (pretty_printer *pp, tree type_a, > > tree type_b, > > > + for (int idx = 0; idx < len_max; idx++) > > + { > > + tree arg_a = idx < len_a ? TREE_VEC_ELT (args_a, idx) : > > NULL; > > + tree arg_b = idx < len_b ? TREE_VEC_ELT (args_b, idx) : > > NULL; > > + if (arg_a == arg_b) > > Same question. If these have to be comparable template type, when > can > len_a and len_b be different? I guess I was thinking about variadic templates. I'll have another look at how these are handled. > > + This is called in phase 2 of pp_format, when it is accumulating > > + a series of formatted chunks. We stash the location of the > > chunk > > + we're meant to have written to, so that we can write to it in > > the > > + m_format_postprocessor hook. */ > > + > > +static void > > +defer_half_of_type_diff (pretty_printer *pp, char spec, tree type, > > If this is called 'phase 2' why not name the function for that, > rather > than 'half'? Maybe I should have used "peer" rather than "half": I'm attempting to refer to one of the two peer types being compared - half of the comparison. > > + case 'H': > > + case 'I': > > + { > > + defer_half_of_type_diff (pp, *spec, next_tree, buffer_ptr, > > verbose); > > Why not tell defer_half_of_type_diff whether it's doing the %H bit or > the %I bit directly, rather than have it also check for H and I? OK. > I've not looked more than cursorily at the non C++ bits. > > nathan Thanks for the input. I'm working on an updated version. Dave
