[PATCH 3/4] (v2) Introduce class edit_context

David Malcolm dmalcolm@redhat.com
Mon Aug 29 18:53:00 GMT 2016


On Sun, 2016-08-28 at 11:13 -0400, Trevor Saunders wrote:
> On Wed, Aug 24, 2016 at 09:13:51PM -0400, David Malcolm wrote:
> > This version removes edit_context::apply_changes and related
> > functionality, retaining the ability to generate diffs.
> > 
> > It also improves error-handling (adding a selftest for this).
> > 
> > gcc/ChangeLog:
> > 	* Makefile.in (OBJS-libcommon): Add edit-context.o.
> > 	* diagnostic-color.c (color_dict): Add "diff-filename",
> > 	"diff-hunk", "diff-delete", and "diff-insert".
> > 	(parse_gcc_colors): Update default value of GCC_COLORS in
> > comment
> > 	to reflect above changes.
> > 	* edit-context.c: New file.
> > 	* edit-context.h: New file.
> > 	* selftest-run-tests.c (selftest::run_tests): Call
> > 	edit_context_c_tests.
> > 	* selftest.h (edit_context_c_tests): New decl.
> > ---
> >  gcc/Makefile.in          |    1 +
> >  gcc/diagnostic-color.c   |    8 +-
> >  gcc/edit-context.c       | 1347
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  gcc/edit-context.h       |   65 +++
> >  gcc/selftest-run-tests.c |    1 +
> >  gcc/selftest.h           |    1 +
> >  6 files changed, 1422 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/edit-context.c
> >  create mode 100644 gcc/edit-context.h
> > 
> > diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> > index f5f3339..506f0d4 100644
> > --- a/gcc/Makefile.in
> > +++ b/gcc/Makefile.in
> > @@ -1561,6 +1561,7 @@ OBJS = \
> >  # Objects in libcommon.a, potentially used by all host binaries
> > and with
> >  # no target dependencies.
> >  OBJS-libcommon = diagnostic.o diagnostic-color.o diagnostic-show
> > -locus.o \
> > +	edit-context.o \
> >  	pretty-print.o intl.o \
> >  	vec.o input.o version.o hash-table.o ggc-none.o memory
> > -block.o \
> >  	selftest.o
> > diff --git a/gcc/diagnostic-color.c b/gcc/diagnostic-color.c
> > index f76c87b..decbe84 100644
> > --- a/gcc/diagnostic-color.c
> > +++ b/gcc/diagnostic-color.c
> > @@ -168,6 +168,10 @@ static struct color_cap color_dict[] =
> >    { "range2", SGR_SEQ (COLOR_FG_BLUE), 6, false },
> >    { "locus", SGR_SEQ (COLOR_BOLD), 5, false },
> >    { "quote", SGR_SEQ (COLOR_BOLD), 5, false },
> > +  { "diff-filename", SGR_SEQ (COLOR_BOLD), 13, false },
> > +  { "diff-hunk", SGR_SEQ (COLOR_FG_CYAN), 9, false },
> > +  { "diff-delete", SGR_SEQ (COLOR_FG_RED), 11, false },
> > +  { "diff-insert", SGR_SEQ (COLOR_FG_GREEN), 11, false },
> >    { NULL, NULL, 0, false }
> >  };
> >  
> > @@ -196,7 +200,9 @@ colorize_stop (bool show_color)
> >  }
> >  
> >  /* Parse GCC_COLORS.  The default would look like:
> > -  
> >  GCC_COLORS='error=01;31:warning=01;35:note=01;36:range1=32:range2=
> > 34;locus=01:quote=01'
> > +   GCC_COLORS='error=01;31:warning=01;35:note=01;36:\
> > +   range1=32:range2=34:locus=01:quote=01:\
> > +   diff-filename=01:diff-hunk=32:diff-delete=31:diff-insert=32'
> >     No character escaping is needed or supported.  */
> >  static bool
> >  parse_gcc_colors (void)
> > diff --git a/gcc/edit-context.c b/gcc/edit-context.c
> > new file mode 100644
> > index 0000000..6b79b45
> > --- /dev/null
> > +++ b/gcc/edit-context.c
> > @@ -0,0 +1,1347 @@
> > +/* Determining the results of applying fix-its.
> > +   Copyright (C) 2016 Free Software Foundation, Inc.
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it
> > under
> > +the terms of the GNU General Public License as published by the
> > Free
> > +Software Foundation; either version 3, or (at your option) any
> > later
> > +version.
> > +
> > +GCC is distributed in the hope that it will be useful, but WITHOUT
> > ANY
> > +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> > License
> > +for more details.
> > +
> > +You should have received a copy of the GNU General Public License
> > +along with GCC; see the file COPYING3.  If not see
> > +<http://www.gnu.org/licenses/>.  */
> > +
> > +#include "config.h"
> > +#include "system.h"
> > +#include "coretypes.h"
> > +#include "line-map.h"
> > +#include "edit-context.h"
> > +#include "pretty-print.h"
> > +#include "diagnostic-color.h"
> > +#include "selftest.h"
> > +
> > +/* This file implements a way to track the effect of fix-its,
> > +   via a class edit_context; the other classes are support classes
> > for
> > +   edit_context.
> > +
> > +   A complication here is that fix-its are expressed relative to
> > coordinates
> > +   in the file when it was parsed, before any changes have been
> > made, and
> > +   so if there's more that one fix-it to be applied, we have to
> > adjust
> > +   later fix-its to allow for the changes made by earlier ones. 
> >  This
> > +   is done by the various "get_effective_column" methods.
> > +
> > +   The "filename" params are required to outlive the edit_context
> > (no
> > +   copy of the underlying str is taken, just the ptr).  */
> > +
> > +/* Forward decls.  class edit_context is declared within edit
> > -context.h.
> > +   The other types are declared here.  */
> > +class edit_context;
> > +class edited_file;
> > +class line_state;
> > +class line_event;
> > +  class insert_event;
> > +  class replace_event;
> > +
> > +/* A struct to hold the params of a print_diff call.  */
> > +
> > +struct diff
> > +{
> > +  diff (pretty_printer *pp, bool show_filenames)
> > +  : m_pp (pp), m_show_filenames (show_filenames) {}
> > +
> > +  pretty_printer *m_pp;
> > +  bool m_show_filenames;
> > +};
> > +
> > +/* The state of one named file within an edit_context: the
> > filename,
> > +   the current content of the file after applying all changes so
> > far, and
> > +   a record of the changes, so that further changes can be applied
> > in
> > +   the correct place.  */
> > +
> > +class edited_file
> > +{
> > + public:
> > +  edited_file (const char *filename);
> > +  bool read_from_file ();
> > +  ~edited_file ();
> 
> nit picking, but I think it would be nice to have the ctor and dtor
> next
> to each other.

(nods), and I think read_from_file could have been made private.  But I
will probably eliminate it as...

> > +  static void delete_cb (edited_file *file);
> > +
> > +  const char *get_filename () const { return m_filename; }
> > +  const char *get_content () const { return m_content; }
> > +
> > +  bool apply_insert (int line, int column, const char *str, int
> > len);
> > +  bool apply_replace (int line, int start_column,
> > +		      int finish_column,
> > +		      const char *replacement_str,
> > +		      int replacement_len);
> > +  int get_effective_column (int line, int column);
> > +
> > +  static int call_print_diff (const char *, edited_file *file,
> > +			      void *user_data)
> > +  {
> > +    diff *d = (diff *)user_data;
> > +    file->print_diff (d->m_pp, d->m_show_filenames);
> > +    return 0;
> > +  }
> > +
> > + private:
> > +  void print_diff (pretty_printer *pp, bool show_filenames);
> > +  void print_line_in_diff (pretty_printer *pp, int line_num);
> > +  line_state *get_line (int line);
> > +  line_state &get_or_insert_line (int line);
> > +  void ensure_capacity (size_t len);
> > +  void ensure_terminated ();
> > +  char *get_line_start (int line_num);
> > +  void ensure_line_start_index ();
> > +  void evict_line_start_index ();
> > +  int get_num_lines ();
> > +
> > +  const char *m_filename;
> > +  char *m_content;
> > +  size_t m_len;
> > +  size_t m_alloc_sz;
> > +  typed_splay_tree<int, line_state *> m_edited_lines;
> > +  auto_vec<int> m_line_start_index;
> 
> have you considered parsing lines when you read the file, and then
> storing a vec<char *> where element I is line I in the file?  Its
> more
> allocations, but when you need to edit a line you'll only need to
> copy
> around that line instead of the whole rest of the file.

...yes, though I'd put the content into class edited_line.  Unedited
lines come from input.c's source cache; we can look up edited lines by
number using the splay tree in class edited_file.   That ought to be
much more efficient that my current "store the whole file in class
edited_file" implementation, and this efficiency would help if we want
to use class edit_context in diagnostic-show-locus.c for printing fix
-it hints, to show the region of the line that's been touched by a fix
-it
(see https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01816.html )

> > +};
> > +
> > +/* A mapping from pre-edit columns to post-edit columns
> > +   within one line of one file.  */
> > +
> > +class line_state
> > +{
> > + public:
> > +  line_state (int line_num) : m_line_num (line_num), m_line_events
> > () {}
> > +  ~line_state ();
> > +  static void delete_cb (line_state *ls);
> > +
> > +  int get_line_num () const { return m_line_num; }
> > +
> > +  int get_effective_column (int orig_column) const;
> > +  void apply_insert (int column, int len);
> > +  void apply_replace (int start, int finish, int len);
> > +
> > + private:
> > +  int m_line_num;
> > +  auto_vec <line_event *> m_line_events;
> > +};
> > +
> > +/* Abstract base class for representing events that have occurred
> > +   on one line of one file.  */
> > +
> > +class line_event
> > +{
> > + public:
> > +  virtual ~line_event () {}
> > +  virtual int get_effective_column (int orig_column) const = 0;
> > +};
> > +
> > +/* Concrete subclass of line_event: an insertion of some text
> > +   at some column on the line.
> > +
> > +   Subsequent events will need their columns adjusting if they're
> > +   are on this line and their column is >= the insertion point. 
> >  */
> > +
> > +class insert_event : public line_event
> > +{
> > + public:
> > +  insert_event (int column, int len) : m_column (column), m_len
> > (len) {}
> > +  int get_effective_column (int orig_column) const FINAL OVERRIDE
> > +  {
> > +    if (orig_column >= m_column)
> > +      return orig_column + m_len;
> > +    else
> > +      return orig_column;
> > +  }
> > +
> > + private:
> > +  int m_column;
> > +  int m_len;
> > +};
> > +
> > +/* Concrete subclass of line_event: the replacement of some text
> > +   betweeen some columns on the line.
> > +
> > +   Subsequent events will need their columns adjusting if they're
> > +   are on this line and their column is >= the finish point.  */
> > +
> > +class replace_event : public line_event
> > +{
> > + public:
> > +  replace_event (int start, int finish, int len) : m_start
> > (start),
> > +    m_finish (finish), m_delta (len - (finish + 1 - start)) {}
> > +
> > +  int get_effective_column (int orig_column) const FINAL OVERRIDE
> > +  {
> > +    if (orig_column >= m_start)
> > +      return orig_column += m_delta;
> > +    else
> > +      return orig_column;
> 
> What happens when orig_column is within the text being replaced?

I think I want to rule that as invalid: that it's not valid to have
overlapping "replace" fixits, and that (ideally) attempts to do so
ought to be rejected within rich_location (they aren't yet rejected at
the moment).

> > +  }
> > +
> > + private:
> > +  int m_start;
> > +  int m_finish;
> > +  int m_delta;
> > +};
> 
> It seems like it would greatly simplify things to merge fixit_insert
> and
> fixit_replace so that an insertion is just a replacement where the
> start
> and end of the replaced text are the same.  That would also have the
> nice effect of making fixit_replace smaller since you wouldn't need
> the
> vtable any more.

That occurred to me.  IIRC clang does this, but their source ranges are
half-open, whereas ours are closed (I don't remember why, only that it
made sense at the time...).  Maybe we could just put a bool into the
combined class, and if an insert, then ignore the range's finish
location.

> > +change_line (edit_context &edit, int line_num)
> 
> I guess this is test only, put I'm not a fan of non const references.

Out of interest, how would you have written it?

> Trev

Thank
Dave



More information about the Gcc-patches mailing list