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