D7525: rust-matchers: improve `Matcher` trait ergonomics
Alphare created this revision. Herald added subscribers: mercurial-devel, kevincox, durin42. Herald added a reviewer: hg-reviewers. REVISION SUMMARY `VisitChildrenSet` has no need to own the set, this will save allocations. The `file_set` return type change is motivated by both ergonomics and... being able to compile code. The `AlwaysMatcher` does not store a `file_set`, which requires it to return an owned `HashSet`, which in turn would change our return type to `Cow<&HgPath>` (lifetimes omitted). This is both un-ergonomic and troublesome for more complex lifetime issues (especially with the upcoming `FileMatcher` in the following patch). REPOSITORY rHG Mercurial BRANCH default REVISION DETAIL https://phab.mercurial-scm.org/D7525 AFFECTED FILES rust/hg-core/src/matchers.rs CHANGE DETAILS diff --git a/rust/hg-core/src/matchers.rs b/rust/hg-core/src/matchers.rs --- a/rust/hg-core/src/matchers.rs +++ b/rust/hg-core/src/matchers.rs @@ -10,21 +10,21 @@ use crate::utils::hg_path::{HgPath, HgPathBuf}; use std::collections::HashSet; -pub enum VisitChildrenSet { +pub enum VisitChildrenSet<'a> { /// Don't visit anything Empty, /// Only visit this directory This, /// Visit this directory and these subdirectories /// TODO Should we implement a `NonEmptyHashSet`? -Set(HashSet), +Set(HashSet<&'a HgPath>), /// Visit this directory and all subdirectories Recursive, } pub trait Matcher { /// Explicitly listed files -fn file_set(&self) -> HashSet<&HgPath>; +fn file_set(&self) -> Option<&HashSet<&HgPath>>; /// Returns whether `filename` is in `file_set` fn exact_match(&self, filename: impl AsRef) -> bool; /// Returns whether `filename` is matched by this matcher @@ -82,8 +82,8 @@ pub struct AlwaysMatcher; impl Matcher for AlwaysMatcher { -fn file_set(&self) -> HashSet<&HgPath> { -HashSet::new() +fn file_set(&self) -> Option<&HashSet<&HgPath>> { +None } fn exact_match(&self, _filename: impl AsRef) -> bool { false To: Alphare, #hg-reviewers Cc: durin42, kevincox, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D7525: rust-matchers: improve `Matcher` trait ergonomics
kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > matchers.rs:87 > +None > } > fn exact_match(&self, _filename: impl AsRef) -> bool { Another option here would just to have a global empty hash set. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7525/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7525 To: Alphare, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D7525: rust-matchers: improve `Matcher` trait ergonomics
Alphare added inline comments. INLINE COMMENTS > kevincox wrote in matchers.rs:87 > Another option here would just to have a global empty hash set. I feel like an `Option` is more explicit and less contrived, but yes. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7525/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7525 To: Alphare, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D7525: rust-matchers: improve `Matcher` trait ergonomics
Closed by commit rHG1bb4e9b02984: rust-matchers: improve `Matcher` trait ergonomics (authored by Alphare). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7525?vs=18401&id=18583 CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7525/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7525 AFFECTED FILES rust/hg-core/src/matchers.rs CHANGE DETAILS diff --git a/rust/hg-core/src/matchers.rs b/rust/hg-core/src/matchers.rs --- a/rust/hg-core/src/matchers.rs +++ b/rust/hg-core/src/matchers.rs @@ -10,21 +10,21 @@ use crate::utils::hg_path::{HgPath, HgPathBuf}; use std::collections::HashSet; -pub enum VisitChildrenSet { +pub enum VisitChildrenSet<'a> { /// Don't visit anything Empty, /// Only visit this directory This, /// Visit this directory and these subdirectories /// TODO Should we implement a `NonEmptyHashSet`? -Set(HashSet), +Set(HashSet<&'a HgPath>), /// Visit this directory and all subdirectories Recursive, } pub trait Matcher { /// Explicitly listed files -fn file_set(&self) -> HashSet<&HgPath>; +fn file_set(&self) -> Option<&HashSet<&HgPath>>; /// Returns whether `filename` is in `file_set` fn exact_match(&self, filename: impl AsRef) -> bool; /// Returns whether `filename` is matched by this matcher @@ -82,8 +82,8 @@ pub struct AlwaysMatcher; impl Matcher for AlwaysMatcher { -fn file_set(&self) -> HashSet<&HgPath> { -HashSet::new() +fn file_set(&self) -> Option<&HashSet<&HgPath>> { +None } fn exact_match(&self, _filename: impl AsRef) -> bool { false To: Alphare, #hg-reviewers, kevincox, pulkit Cc: durin42, kevincox, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel