On Tue, 2017-05-09 at 10:52 -0400, Nathan Sidwell wrote:
> On 05/09/2017 10:07 AM, David Malcolm wrote:
>
>
> > +static const char *
> > +type_to_string_with_compare (tree type, tree peer, bool verbose,
> > + bool show_color)
> > +{
>
> > + for (int idx = 0; idx < len_max; idx++)
> > + {
> ...
> > + if (idx + 1 < len_max)
> > + pp_character (pp, ',');
> > + }
>
> My favorite idiom for this is to place the test at the beginning of
> the
> loop and avoid an extra loop value check. (perhaps optimizers are
> smart
> enough to tell that 'idx + 1 < max' and 'idx++, idx < max' are the
> same
> these days)
>
> if (idx)
> pp_character (pp, ',');
Done.
> > +static void
> > +print_template_tree_comparison (pretty_printer *pp, tree type_a,
> > tree type_b,
> > + bool verbose, int indent)
>
> It looks to me that type_to_string_with_compare and
> print_template_tree_comparison are doing very nearly the same thing,
> with a little formatting difference. How hard would it be to have
> them
> forward to a worker function?
Done: I merged their bodies into a new function -
print_template_differences.
> > + unsigned int chunk_idx;
> > + for (chunk_idx = 0; args[chunk_idx]; chunk_idx++)
> > + ;
>
> I have a fondness for putting 'continue;' as the body of such empty
> loops. Dunno if that's style-compliant though.
Dunno either. I left this untouched.
> > +void
> > +cxx_format_postprocessor::handle (pretty_printer *pp)
> > +{
> > + /* If we have one of %H and %I, the other should have
> > + been present. */
> > + if (m_type_a.m_tree || m_type_b.m_tree)
> > + {
> > + gcc_assert (m_type_a.m_tree);
> > + gcc_assert (m_type_b.m_tree);
> > + }
>
> > + if (m_type_a.m_tree && m_type_b.m_tree)
> As you fall into this. Why not simply
> if (m_type_a.m_tree || m_type_b.m_tree)
> { do stuff that will seg fault if one's null }
Done.
> > + gcc_assert (type_a.m_tree);
>
> And these asserts are confusing, because some, at least, seem to be
> checking the if condition.
>
> > + gcc_assert (type_a.m_buffer_ptr);
> > + gcc_assert (type_b.m_tree);
> > + gcc_assert (type_b.m_buffer_ptr);
>
>
> Generally the C++ bits look good, and my style comments are FWIW not
> obligatory.
> 1) is it possible to commonize the two functions I mention
> 2) cleanup the unnecessary asserts in
> cxx_format_postprocessor::handle
Done.
> non-c++ bits not reviewed.
>
> nathan
I split out the non-c++ bits into a separate patch.
v3 of the patch kit is thus three parts:
[1/3] Non-C++ parts of template type diff printing
I believe I can self-approve this part
[2/3] (v3) C++ template type diff printing
Updated as above.
[3/3] (v3) Use %qH and %qI throughout C++ frontend
Mostly mechanical
I've successfully bootstrapped®rtested the combination of the
three patches on x86_64-pc-linux-gnu.
Are patches 2 and 3 OK for trunk?
Thanks
Dave
gcc/c-family/c-format.c | 2 +-
gcc/c-family/c.opt | 8 +
gcc/c/c-objc-common.c | 5 +-
gcc/cp/call.c | 40 +-
gcc/cp/cvt.c | 18 +-
gcc/cp/error.c | 453 ++++++++++++++++++++-
gcc/cp/typeck.c | 22 +-
gcc/cp/typeck2.c | 6 +-
gcc/diagnostic-color.c | 6 +-
gcc/doc/invoke.texi | 50 ++-
gcc/fortran/error.c | 5 +-
gcc/pretty-print.c | 11 +-
gcc/pretty-print.h | 20 +-
gcc/testsuite/g++.dg/plugin/plugin.exp | 3 +
.../show-template-tree-color-no-elide-type.C | 30 ++
.../g++.dg/plugin/show-template-tree-color.C | 30 ++
.../plugin/show_template_tree_color_plugin.c | 38 ++
.../g++.dg/template/show-template-tree-2.C | 118 ++++++
.../g++.dg/template/show-template-tree-3.C | 37 ++
.../g++.dg/template/show-template-tree-4.C | 95 +++++
.../template/show-template-tree-no-elide-type.C | 24 ++
gcc/testsuite/g++.dg/template/show-template-tree.C | 51 +++
gcc/tree-diagnostic.c | 3 +-
gcc/tree-diagnostic.h | 2 +-
24 files changed, 1018 insertions(+), 59 deletions(-)
create mode 100644
gcc/testsuite/g++.dg/plugin/show-template-tree-color-no-elide-type.C
create mode 100644 gcc/testsuite/g++.dg/plugin/show-template-tree-color.C
create mode 100644
gcc/testsuite/g++.dg/plugin/show_template_tree_color_plugin.c
create mode 100644 gcc/testsuite/g++.dg/template/show-template-tree-2.C
create mode 100644 gcc/testsuite/g++.dg/template/show-template-tree-3.C
create mode 100644 gcc/testsuite/g++.dg/template/show-template-tree-4.C
create mode 100644
gcc/testsuite/g++.dg/template/show-template-tree-no-elide-type.C
create mode 100644 gcc/testsuite/g++.dg/template/show-template-tree.C
--
1.8.5.3
