Re: [PATCH 01/02] PR/62314: add ability to add fixit-hints

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2015-11-18 at 14:57 -0700, Jeff Law wrote:
> On 11/10/2015 09:35 AM, David Malcolm wrote:
> > This patch adds the ability to add "fix-it hints" to a rich_location,
> > which will be displayed when the corresponding diagnostic is printed.
> >
> > It does not actually add any fix-it hints (that comes in the second
> > patch), but it adds test coverage of the machinery and printing,
> > by using the existing diagnostic_plugin_test_show_locus to inject
> > some meaningless fixit hints, and to verify the output.
> >
> > For now, add a nasty linker kludge in layout::print_any_fixits for
> > the sake of diagnostic_plugin_test_show_locus.
> >
> > Successfully bootstrapped&regrtested the pair of patches on
> > x86_64-pc-linux-gnu (on top of the 10-patch diagnostics kit).
> >
> > OK for trunk?
> >
> > gcc/ChangeLog:
> > 	PR/62314
> > 	* diagnostic-show-locus.c (colorizer::set_fixit_hint): New.
> > 	(class layout): Update comment
> > 	(layout::print_any_fixits): New method.
> > 	(layout::move_to_column): New method.
> > 	(diagnostic_show_locus): Add call to layout.print_any_fixits.
> >
> > gcc/testsuite/ChangeLog:
> > 	PR/62314
> > 	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-bw.c
> > 	(test_fixit_insert): New.
> > 	(test_fixit_remove): New.
> > 	(test_fixit_replace): New.
> > 	* gcc.dg/plugin/diagnostic-test-show-locus-ascii-color.c
> > 	(test_fixit_insert): New.
> > 	(test_fixit_remove): New.
> > 	(test_fixit_replace): New.
> > 	* gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
> > 	(test_show_locus): Add tests of rendering fixit hints.
> >
> > libcpp/ChangeLog:
> > 	PR/62314
> > 	* include/line-map.h (source_range::intersects_line_p): New
> > 	method.
> > 	(rich_location::~rich_location): New.
> > 	(rich_location::add_fixit_insert): New method.
> > 	(rich_location::add_fixit_remove): New method.
> > 	(rich_location::add_fixit_replace): New method.
> > 	(rich_location::get_num_fixit_hints): New accessor.
> > 	(rich_location::get_fixit_hint): New accessor.
> > 	(rich_location::MAX_FIXIT_HINTS): New constant.
> > 	(rich_location::m_num_fixit_hints): New field.
> > 	(rich_location::m_fixit_hints): New field.
> > 	(class fixit_hint): New class.
> > 	(class fixit_insert): New class.
> > 	(class fixit_remove): New class.
> > 	(class fixit_replace): New class.
> > 	* line-map.c (source_range::intersects_line_p): New method.
> > 	(rich_location::rich_location): Add initialization of
> > 	m_num_fixit_hints to both ctors.
> > 	(rich_location::~rich_location): New.
> > 	(rich_location::add_fixit_insert): New method.
> > 	(rich_location::add_fixit_remove): New method.
> > 	(rich_location::add_fixit_replace): New method.
> > 	(fixit_insert::fixit_insert): New.
> > 	(fixit_insert::~fixit_insert): New.
> > 	(fixit_insert::affects_line_p): New.
> > 	(fixit_remove::fixit_remove): New.
> > 	(fixit_remove::affects_line_p): New.
> > 	(fixit_replace::fixit_replace): New.
> > 	(fixit_replace::~fixit_replace): New.
> > 	(fixit_replace::affects_line_p): New.
> 
> > +
> > +  /* Nasty workaround to convince the linker to add
> > +      rich_location::add_fixit_insert
> > +      rich_location::add_fixit_remove
> > +      rich_location::add_fixit_replace
> > +     to cc1 for use by diagnostic_plugin_test_show_locus,
> > +     before anything in cc1 is using them.
> > +
> > +     This conditional should never hold, but hopefully the compiler can't
> > +     figure that out.  */
> > +  if ('.' == global_dc->caret_chars[0])
> > +    {
> > +      rich_location dummy (line_table, UNKNOWN_LOCATION);
> > +      dummy.add_fixit_insert (UNKNOWN_LOCATION, "");
> > +      dummy.add_fixit_remove
> > +	(source_range::from_location (UNKNOWN_LOCATION));
> > +      dummy.add_fixit_replace
> > +	(source_range::from_location (UNKNOWN_LOCATION), "");
> > +    }
> > +}
> So "the kludge" is presumably going to be removed based on later 
> comments in this patch's thread.

Yes.

> What is the purpose of the #if 0 code in the various tests?  Did you 
> mean to leave those in?

Presumably you're referring to the bodies of the functions
  test_fixit_insert
  test_fixit_remove
  test_fixit_replace
within:
  gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c
  gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c

where the bodies are purely of the form:
{
  #if 0
  some code, containing dejagnu directives.
  #endif
}

Although this looks weird, it's deliberate, and follows the pattern
earlier in those test files: the diagnostics are injected by the plugin,
not by cc1.  The plugin gives us a way of unit-testing how
diagnostic_show_locus handles the various ways of calling into the
diagnostic API, isolating it from the details of any particular real
diagnostic in cc1, and from the details of how to get real
location/range information.

Hence we inject calls to the diagnostic API via the plugin, and the
bodies of the function could be anything - but I chose them to give
representative-looking diagnostics.  In fact, in
test_caret_on_leading_whitespace we have a fragment of Fortran code in a
C file (since this was a regression test for an issue I saw printing
Fortran diagnostics during development of the new
diagnostic_show_locus).

Hope this makes sense.  (FWIW there's a comment about this at the top of
diagnostic_plugin_test_show_locus.c, though of course that's not visible
in the patch under review).

Hence the testcase gives some examples of what uses of the 3 kinds of
fix-its might look like.  To actually implement those fix-its for those
diagnostics would be separate patches (fwiw I have some followups
already posted that update the levenshtein/spelling suggestion thing to
issue fix-its for the suggestions, but they'll have bit-rotted; I'll
update them and repost them).

(Ultimately we could have some kind of option to emit the fixit in
machine-parsable form, so that e.g. an IDE can offer to directly apply
the change; again, I see that as a followup).


> I think this is probably OK.  Though I am concerned that with blobs of 
> the tests commented out that it's not being tested as thoroughly as you 
> think it has :-)

Does the above address your concerns?  (Joesph already approved the
other patch)

Dave