This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7525

rust-matchers: improve `Matcher` trait ergonomics
ClosedPublic

Authored by Alphare on Nov 29 2019, 12:56 PM.

Details

Reviewers
kevincox
pulkit
Group Reviewers
hg-reviewers
Commits
rHG1bb4e9b02984: rust-matchers: improve `Matcher` trait ergonomics
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).

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Nov 29 2019, 12:56 PM
kevincox accepted this revision.Dec 2 2019, 8:09 AM
kevincox added inline comments.
rust/hg-core/src/matchers.rs
87

Another option here would just to have a global empty hash set.

Alphare added inline comments.Dec 2 2019, 9:20 AM
rust/hg-core/src/matchers.rs
87

I feel like an Option is more explicit and less contrived, but yes.

pulkit accepted this revision.Dec 10 2019, 10:13 AM
This revision is now accepted and ready to land.Dec 10 2019, 10:13 AM
Alphare added a commit: rHG1bb4e9b02984: rust-matchers: improve `Matcher` trait ergonomics.Dec 10 2019, 11:17 AM
Closed by commit rHG1bb4e9b02984: rust-matchers: improve `Matcher` trait ergonomics (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/matchers.rs (10 lines)

Diff 18401

rust/hg-core/src/matchers.rs

// matchers.rs // matchers.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Structs and types for matching files and directories. //! Structs and types for matching files and directories.
use crate::utils::hg_path::{HgPath, HgPathBuf}; use crate::utils::hg_path::{HgPath, HgPathBuf};
use std::collections::HashSet; use std::collections::HashSet;
pub enum VisitChildrenSet { pub enum VisitChildrenSet<'a> {
/// Don't visit anything /// Don't visit anything
Empty, Empty,
/// Only visit this directory /// Only visit this directory
This, This,
/// Visit this directory and these subdirectories /// Visit this directory and these subdirectories
/// TODO Should we implement a `NonEmptyHashSet`? /// TODO Should we implement a `NonEmptyHashSet`?
Set(HashSet<HgPathBuf>), Set(HashSet<&'a HgPath>),
/// Visit this directory and all subdirectories /// Visit this directory and all subdirectories
Recursive, Recursive,
} }
pub trait Matcher { pub trait Matcher {
/// Explicitly listed files /// Explicitly listed files
fn file_set(&self) -> HashSet<&HgPath>; fn file_set(&self) -> Option<&HashSet<&HgPath>>;
/// Returns whether `filename` is in `file_set` /// Returns whether `filename` is in `file_set`
fn exact_match(&self, filename: impl AsRef<HgPath>) -> bool; fn exact_match(&self, filename: impl AsRef<HgPath>) -> bool;
/// Returns whether `filename` is matched by this matcher /// Returns whether `filename` is matched by this matcher
fn matches(&self, filename: impl AsRef<HgPath>) -> bool; fn matches(&self, filename: impl AsRef<HgPath>) -> bool;
/// Decides whether a directory should be visited based on whether it /// Decides whether a directory should be visited based on whether it
/// has potential matches in it or one of its subdirectories, and /// has potential matches in it or one of its subdirectories, and
/// potentially lists which subdirectories of that directory should be /// potentially lists which subdirectories of that directory should be
/// visited. This is based on the match's primary, included, and excluded /// visited. This is based on the match's primary, included, and excluded
fn is_exact(&self) -> bool; fn is_exact(&self) -> bool;
} }
/// Matches everything. /// Matches everything.
#[derive(Debug)] #[derive(Debug)]
pub struct AlwaysMatcher; pub struct AlwaysMatcher;
impl Matcher for AlwaysMatcher { impl Matcher for AlwaysMatcher {
fn file_set(&self) -> HashSet<&HgPath> { fn file_set(&self) -> Option<&HashSet<&HgPath>> {
HashSet::new() None
} }
kevincoxUnsubmitted
Not Done

Another option here would just to have a global empty hash set.

kevincox: Another option here would just to have a global empty hash set.
AlphareAuthorUnsubmitted
Done

I feel like an Option is more explicit and less contrived, but yes.

Alphare: I feel like an `Option` is more explicit and less contrived, but yes.
fn exact_match(&self, _filename: impl AsRef<HgPath>) -> bool { fn exact_match(&self, _filename: impl AsRef<HgPath>) -> bool {
false false
} }
fn matches(&self, _filename: impl AsRef<HgPath>) -> bool { fn matches(&self, _filename: impl AsRef<HgPath>) -> bool {
true true
} }
fn visit_children_set( fn visit_children_set(
&self, &self,