Patchwork D7525: rust-matchers: improve `Matcher` trait ergonomics

login
register
mail settings
Submitter phabricator
Date Nov. 29, 2019, 5:57 p.m.
Message ID <differential-rev-PHID-DREV-snbatve5e7acjf4d6o6r-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43531/
State Superseded
Headers show

Comments

phabricator - Nov. 29, 2019, 5:57 p.m.
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




To: Alphare, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - Dec. 2, 2019, 1:09 p.m.
kevincox added inline comments.
kevincox accepted this revision.

INLINE COMMENTS

> matchers.rs:87
> +        None
>      }
>      fn exact_match(&self, _filename: impl AsRef<HgPath>) -> 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
phabricator - Dec. 2, 2019, 2:20 p.m.
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

Patch

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<HgPathBuf>),
+    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<HgPath>) -> 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<HgPath>) -> bool {
         false