D6271: rust-filepatterns: add a Rust implementation of pattern-related utils
Alphare (Raphaël Gomès)
phabricator at mercurial-scm.org
Wed Apr 24 05:49:36 EDT 2019
Alphare added inline comments.
> kevincox wrote in filepatterns.rs:18
> Why not use https://docs.rs/regex/1.1.6/regex/fn.escape.html? If you are worried about performance you can use https://docs.rs/regex-syntax/0.6.6/regex_syntax/fn.escape_into.html which is quite fast.
I think I hit the same problem as the person in this issue: `https://github.com/rust-lang/regex/issues/451`. It's a hassle to escape bytes using the `regex` crate, and this felt like a good enough replacement.
> kevincox wrote in filepatterns.rs:51
> I suspect this `.iter()` isn't required.
It is required since `repl` is `&&[u8]`, which is not an iterator.
> kevincox wrote in filepatterns.rs:61
> input.iter().skip(1).position(|b| *b == b']')
This has a different behavior, since `position` expects a boolean return value in the closure for each element of the iterator.
> kevincox wrote in filepatterns.rs:226
> This function appears to be way more complicated then it needs to be. I have tried to highlight the changes in comments below but I have a full version at https://rust.godbolt.org/z/08MrDP. I haven't actually run the code but I think my version has the same functionality as this one.
While your version is just plain better, removing the regex-based comment escape breaks support for comments in `test-hgignore.t`, so this will still be necessary.
> kevincox wrote in lib.rs:47
> This seems like overkill to me. I would either remove it or use `pub struct LineNumber(pub usize)` if you want to do this for type safety. However I don't think type safety is really needed here so I would just remove the alias.
I would argue it's here for readability's sake. I used to have `usize`, but was advised to put an alias here because it would lessen mental overhead to read the function signatures. I'll remove it If you disagree.
To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
More information about the Mercurial-devel