[PATCH 1/2] C++ template type diff printing

[David Malcolm]

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