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.

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6271

To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel


More information about the Mercurial-devel mailing list