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
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.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 18583

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,