This is an archive of the discontinued Mercurial Phabricator instance.

rust-filepatterns: improve API and robustness for pattern files parsing
ClosedPublic

Authored by Alphare on Jan 16 2020, 5:22 AM.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Jan 16 2020, 5:22 AM
Alphare updated this revision to Diff 19356.Jan 16 2020, 6:27 AM
kevincox added inline comments.Jan 16 2020, 10:17 AM
rust/hg-core/src/filepatterns.rs
220

It might be clearer to do bytes.iter().take_while(|b| b == sep).count()

237

Personally I think this would be more clear as a for loop.

250

Should this be part of HgPathBuf or is this normalization specific to the file pattern matching in some way?

Alphare marked an inline comment as done.Jan 16 2020, 1:41 PM
Alphare added inline comments.
rust/hg-core/src/filepatterns.rs
220

Indeed. Also added a comment to explain the reasoning.

237

I'm not sure I agree. On the basis that "Rust might optimize iterators better than for loops" I vote to keep it that way.

250

For now this is pretty specific, but it might need to be moved later when more code paths use it.

kevincox accepted this revision.Jan 16 2020, 2:15 PM
kevincox added inline comments.
rust/hg-core/src/filepatterns.rs
227

Can you move this condition into a .filter()?

237

If you think it' is clearer that's fine. But I'm curious where "Rust might optimize iterators better than for loops" comes from. I did a quick test and it looks like the for loop actually generates slightly better code https://rust.godbolt.org/z/vFAWyZ

Alphare marked 2 inline comments as done.Jan 16 2020, 3:19 PM
Alphare added inline comments.
rust/hg-core/src/filepatterns.rs
237

I might be using godbolt wrong but it looks like you are using different versions of rustc. When using 1.40 for both sides, I don't see any clear win on either side... but I'm not fluent in assembly either. As I'm sure you'll agree, this function is unlikely to show up in future profiles anyway.

kevincox added inline comments.Jan 16 2020, 3:35 PM
rust/hg-core/src/filepatterns.rs
237

Oops. My mistake. But I think we agree that readability is the top priority (until profiles say otherwise)

Alphare updated this revision to Diff 19384.Jan 16 2020, 4:50 PM
Alphare updated this revision to Diff 20038.Feb 10 2020, 10:46 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.