Panics when resizing #1408
Panics when resizing #1408
Conversation
helix-tui/src/buffer.rs
Outdated
| pub fn get(&self, x: u16, y: u16) -> Option<&Cell> { | ||
| self.index_of_opt(x, y).map(|i| &self.content[i]) | ||
| } | ||
|
|
||
| /// Returns a mutable reference to Cell at the given coordinates | ||
| pub fn get_mut(&mut self, x: u16, y: u16) -> &mut Cell { | ||
| let i = self.index_of(x, y); | ||
| &mut self.content[i] | ||
| pub fn get_mut(&mut self, x: u16, y: u16) -> Option<&mut Cell> { | ||
| self.index_of_opt(x, y).map(|i| &mut self.content[i]) | ||
| } |
There was a problem hiding this comment.
I'd rather see you use the original method and then use self.content.get(i). That way you're not doing bounds checks twice (once in index_of_opt, once internally in self.content[]).
There was a problem hiding this comment.
I don't think that would work. There is no way to avoid bounds checks on a Vec (to my knowledge) and the check performed by index_of is actually different:
Say the buffer is of size (2, 2), then asking for out-of-bounds cell at (2, 0) will compute index=2, which is in bounds of vec but correspond to incorrect cell at (0,1).
(We could accept that and tolerate that buffer can return bad cells when out of bounds, but I don't think optimizing performance for this kind of double checks is worth it)
There was a problem hiding this comment.
Well there's unsafe get_unchecked() if you previously validate the index.
We inherited the Buffer implementation from https://github.com/fdehau/tui-rs/ and I think this explains why it panics and only uses a debug_assert! rather than Option: Buffer::get_mut is a hot path that gets called a lot on every render. On frequent re-renders the slowdown will be noticeable.
There was a problem hiding this comment.
Do you want me to use get_unchecked or do you overall disagree with the proposed solution?
I think it's best to have safe functions in Buffer at the cost of (what I think to be) unnoticeable performance gains. I could be wrong, but I think the terminal emulator is more responsible of refresh latency than the editor.
Also, 'premature optimization is the root of all evil' :) Helix is in alpha state, it should be stable and feature-full before being optimized for speed / latency.
There was a problem hiding this comment.
Looking at this again, I think the change is OK since now in most cases we use [] instead of get_mut anyway (like set_stringn).
FIxes #1403
The editor crashes when the window is very small. Two reasons:
Buffer::getandBuffer::get_mutto return an Option, likestd::VecIndexandIndexMutfor Buffer with a pair of coordinates, i.e.buf[(x, y)]Buffer::getandBuffer.get_mutwith the Index, which is known to panic. Makes code clearer with the intent not to perform bounds checks.