implement translate_offset_with_edit #325
Conversation
| let mut sorted_edits: Vec<&AtomTextEdit> = Vec::with_capacity(edits.len()); | ||
| for edit in edits { | ||
| let insert_index = | ||
| sorted_edits.upper_bound_by_key(&edit.delete.start(), |x| x.delete.start()); |
There was a problem hiding this comment.
The TextEdit struct grantees that its AtomicEdits are disjoint and sorted. Can we accept an TextEdit here instead of the slice and avoid the sorting step?
There was a problem hiding this comment.
Yes that makes sense. Although reviewing the code, when translate_offset_with_edit is called we don't have a TextEdit. Here https://github.com/vemoo/rust-analyzer/blob/0156a538089828340a823ed02da8970bf4f1175b/crates/ra_lsp_server/src/conv.rs#L204 the edits come from SourceFileNodeEdit https://github.com/vemoo/rust-analyzer/blob/0156a538089828340a823ed02da8970bf4f1175b/crates/ra_analysis/src/lib.rs#L124
Should we create a new TextEdit before calling translate_offset_with_edit or should SourceFileNodeEdit have a TextEdit?
There was a problem hiding this comment.
I've changed that to use TextEdit a couple of days ago :-)
ideally, we should not expose AtomicEdit at all, but we need that precisely to be able to construct LSP edits.
There was a problem hiding this comment.
So if you rebase this PR onto the current master it should be ready to go?
There was a problem hiding this comment.
Ok, I will do that later today
There was a problem hiding this comment.
I've rebased and changed translate_offset_with_edit to accept TextEdit.
| line_index::to_line_col(&text, offset) | ||
| } | ||
|
|
||
| fn edit_text(pre_edit_text: &str, mut edits: Vec<AtomTextEdit>) -> String { |
There was a problem hiding this comment.
This could be TextEdit::apply I think?
There was a problem hiding this comment.
Yes, I will change it
|
Overall, LGTM, however I feel slightly uncomfortable about the inclusion of becnhmarks: it adds a bit of code and quite a few dependencies, but I am not sure that, in the grand scheme of things, translating offsets takes a huge amount of time. I think currently it makes sense to focus on correctness; for performance, we really should start with high level "whole-program" benchmarks to figure out what exactly needs to be optimize. Would do you think about removing benchmark support for now? |
|
Yeah, you are right, I'll remove the benchmark. It was mostly so I could see it wasn't slower than the naive method. |
todo: cleanup, simplify handle columns
the main thing is we iterate over inserted newlines at once for each edit
vemoo
force-pushed
the
ra-text-edit-refactor
branch
from
b16c684
to
863ed19
Compare
rename newline to step where applicable
|
bors r+ Thanks! |
325: implement translate_offset_with_edit r=matklad a=vemoo - Implement `translate_offset_with_edit` to resolve #105 - Add proptest impls for text, offsets and edits and use them in tests for `translate_offset_with_edit` and `LineIndex` - Added benchmark for `translate_offset_with_edit` Co-authored-by: Bernardo <berublan@gmail.com>
Build succeeded |
translate_offset_with_editto resolve FixTryConvWithfor SourceEdit #105translate_offset_with_editandLineIndextranslate_offset_with_edit