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
