On Mon, Aug 29, 2016 at 02:53:43PM -0400, David Malcolm wrote:
> 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:
> > > +
> > > + 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 )
yeah, that does sound even better.
> > > +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).
yeah, that is the simple solution ;)
>
> > > + }
> > > +
> > > + 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.
yeah, that is a complication to be thought about.
>
> > > +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?
probably just using a pointer. the nonnull template in the GSL would be
nice, but I'm not sure if I care enough to backport it to C++98.
Trev
>
> > Trev
>
> Thank
> Dave
