nushell / reedline

A feature-rich line editor - powering Nushell
https://docs.rs/reedline/
MIT License
549 stars 154 forks source link

Suggestion about is_valid function of LineBuffer struct #760

Open Laurent45 opened 9 months ago

Laurent45 commented 9 months ago

A little suggestion. It seems like first check and second check do the same check implicitly. Is there a case where the first check is false and the second true ? What do you think ?

pub fn is_valid(&self) -> bool {
        self.lines.is_char_boundary(self.insertion_point())  // first check
            && (self
                .lines
                .grapheme_indices(true)
                .any(|(i, _)| i == self.insertion_point())
                || self.insertion_point() == self.lines.len())    // second check
            && std::str::from_utf8(self.lines.as_bytes()).is_ok()
    }
sholderbach commented 9 months ago

You are correct that if the second test (grapheme cluster boundary) is true the first one (UTF-8 char boundary) will always be true.

The first check is local and only needs to check the byte at the index (or length), while the follow-on second check requires us to traverse the string. Without benchmarking with real data (that is most likely valid), there could be a questionable argument about short-circuiting saving us the work for the second check. Given that we expect valid data anyways, we could probably elide the first check.