an-cabal / an-rope

an rope data structure
https://docs.rs/an-rope
MIT License
11 stars 2 forks source link

Make Ropes fully parameterised with Metrics #64

Closed hawkw closed 7 years ago

hawkw commented 7 years ago

This PR extends the existing Rope metrics system to make all Rope methods optionally parameterised with Metrics. This allows you to split, insert, or delete Ropes on character, line, or grapheme indices, without adding a whole bunch of new, wordy method names like split_on_grapheme_index and so on.

For example, if I wanted to delete characters 5 to 15 from rope r, I could say:

r.delete(5..15);

but if I actually wanted to delete graphemes 5 to 15, I could instead say:

r.delete(Grapheme(5)..Grapheme(15));

or even

r.delete(Line(5)..Line(15));

Note how the use of a newtype to represent indices along a metric, allowing for nicely natural language-like syntax (e.g. Line(5), which is pretty close to the English "line 5").

Metric is implemented for usize, which is the "default" char-based metric. This way, this PR is backwards compatible with most of our existing code, and Metric-based operations nicely default to char-based indexing when used with number literals.

Things that are still needed:

hawkw commented 7 years ago

Performance is very slightly worse than when we used to only support char-based indexing, by a constant factor of about 200 ms. I think there's some room for optimisation in there, since given that Metric is almost entirely trait based, it should be a zero-cost abstraction. Note that benchmark results is significantly better than the current master (8dd43f3d09444fe1ed69e57fc6d2e23204b4386f), where ALL indexing is along Graphemes.

hawkw commented 7 years ago

This also still needs support for tendril – it should be trivial to add Measured implementations for StrTendrils. Currently I haven't added that yet, since tendril support was already broken on the master this branch was forked from (see #63).

twisted-pear commented 7 years ago

this is awesomesauce, will check it out as soon as i can, but great work

hawkw commented 7 years ago

I think this should push either a patch (v0.0.4) or a minor version (v0.1.0) when it hits master?

hawkw commented 7 years ago

I suspect that a lot of the boilerplate in here could be reduced with the newtype-derive crate...

hawkw commented 7 years ago

@cjm00: I'm working on adding docs to get this ready to merge, but I've hit a couple of bugs that still need to be fixed, so it might take slightly longer.

hawkw commented 7 years ago

It looks like 40964669918197fa92b645769b3766abe2f75c67 also fixes #65; that was supposed to go on master but I committed it to my dev branch by mistake :sweat:

codecov-io commented 7 years ago

Current coverage is 94.43% (diff: 100%)

No coverage report found for master at 8dd43f3.

Powered by Codecov. Last update 8dd43f3...d808848