From de6d418d42e078ca8dd4541d9733d99a3541edd6 Mon Sep 17 00:00:00 2001 From: cyqsimon <28627918+cyqsimon@users.noreply.github.com> Date: Sun, 5 Nov 2023 00:08:05 +0800 Subject: [PATCH] Remove `BuiltinMatcher` enum Explanation added as comments in code Using plain `Lazy>` is just better --- build/syntax_mapping.rs | 11 +++--- src/syntax_mapping.rs | 82 ++++++++++++++++++++++++++--------------- 2 files changed, 58 insertions(+), 35 deletions(-) diff --git a/build/syntax_mapping.rs b/build/syntax_mapping.rs index ccbadbd8..cecf8ef5 100644 --- a/build/syntax_mapping.rs +++ b/build/syntax_mapping.rs @@ -41,7 +41,7 @@ impl MappingTarget { #[derive(Clone, Debug, DeserializeFromStr)] /// A single matcher. /// -/// Corresponds to `syntax_mapping::BuiltinMatcher`. +/// Codegen converts this into a `Lazy`. struct Matcher(Vec); /// Parse a matcher. /// @@ -110,13 +110,12 @@ impl Matcher { let MatcherSegment::Text(ref s) = self.0[0] else { unreachable!() }; - format!(r###"BuiltinMatcher::Fixed(r#"{s}"#)"###) + format!(r###"Lazy::new(|| Some(build_matcher_fixed(r#"{s}"#)))"###) } // parser logic ensures that this case can only happen when there are dynamic segments _ => { - let segments_codegen = self.0.iter().map(MatcherSegment::codegen).join(", "); - let closure = format!("|| build_glob_string(&[{segments_codegen}])"); - format!("BuiltinMatcher::Dynamic(Lazy::new({closure}))") + let segs = self.0.iter().map(MatcherSegment::codegen).join(", "); + format!(r###"Lazy::new(|| build_matcher_dynamic(&[{segs}]))"###) } } } @@ -174,7 +173,7 @@ impl MappingList { let len = array_items.len(); format!( - "static BUILTIN_MAPPINGS: [(BuiltinMatcher, MappingTarget); {len}] = [\n{items}\n];", + "static BUILTIN_MAPPINGS: [(Lazy>, MappingTarget); {len}] = [\n{items}\n];", items = array_items.join(",\n") ) } diff --git a/src/syntax_mapping.rs b/src/syntax_mapping.rs index 2b1f811f..8cc009a4 100644 --- a/src/syntax_mapping.rs +++ b/src/syntax_mapping.rs @@ -15,38 +15,60 @@ include!(concat!( "/codegen_static_syntax_mappings.rs" )); -/// A glob matcher generated from analysing the matcher string at compile time. +// The defined matcher strings are analysed at compile time and converted into +// lazily-compiled `GlobMatcher`s. This is so that the string searches are moved +// from run time to compile time, thus improving startup performance. +// +// To any future maintainer (including possibly myself) wondering why there is +// not a `BuiltinMatcher` enum that looks like this: +// +// ``` +// enum BuiltinMatcher { +// Fixed(&'static str), +// Dynamic(Lazy>), +// } +// ``` +// +// Because there was. I tried it and threw it out. +// +// Naively looking at the problem from a distance, this may seem like a good +// design (strongly typed etc. etc.). It would also save on compiled size by +// extracting out common behaviour into functions. But while actually +// implementing the lazy matcher compilation logic, I realised that it's most +// convenient for `BUILTIN_MAPPINGS` to have the following type: +// +// `[(Lazy>, MappingTarget); N]` +// +// The benefit for this is that operations like listing all builtin mappings +// would be effectively memoised. The caller would not have to compile another +// `GlobMatcher` for rules that they have previously visited. +// +// Unfortunately, this means we are going to have to store a distinct closure +// for each rule anyway, which makes a `BuiltinMatcher` enum a pointless layer +// of indirection. +// +// In the current implementation, the closure within each generated rule simply +// calls either `build_matcher_fixed` or `build_matcher_dynamic`, depending on +// whether the defined matcher contains dynamic segments or not. + +/// Compile a fixed glob string into a glob matcher. /// -/// This is so that the string searches are moved from run time to compile time, -/// thus improving startup performance. -#[derive(Debug)] -enum BuiltinMatcher { - /// A plaintext matcher. - Fixed(&'static str), - /// A matcher that needs dynamic environment variable replacement. - /// - /// Evaluates to `None` when any environment variable replacement fails. - Dynamic(Lazy>), -} -impl BuiltinMatcher { - /// Finalise into a glob matcher. - /// - /// Returns `None` if any environment variable replacement fails (only - /// possible for dynamic matchers). - fn to_glob_matcher(&self) -> Option { - let glob_str = match self { - Self::Fixed(s) => *s, - Self::Dynamic(s) => s.as_ref()?.as_str(), - }; - Some(make_glob_matcher(glob_str).expect("A builtin glob matcher failed to compile")) - } +/// A failure to compile is a fatal error. +/// +/// Used internally by `Lazy`'s lazy evaluation closure. +fn build_matcher_fixed(from: &str) -> GlobMatcher { + make_glob_matcher(from).expect("A builtin fixed glob matcher failed to compile") } /// Join a list of matcher segments to create a glob string, replacing all -/// environment variables. Returns `None` if any replacement fails. +/// environment variables, then compile to a glob matcher. /// -/// Used internally by `BuiltinMatcher::Dynamic`'s lazy evaluation closure. -fn build_glob_string(segs: &[MatcherSegment]) -> Option { +/// Returns `None` if any replacement fails, or if the joined glob string fails +/// to compile. +/// +/// Used internally by `Lazy`'s lazy evaluation closure. +fn build_matcher_dynamic(segs: &[MatcherSegment]) -> Option { + // join segments let mut buf = String::new(); for seg in segs { match seg { @@ -57,12 +79,14 @@ fn build_glob_string(segs: &[MatcherSegment]) -> Option { } } } - Some(buf) + // compile glob matcher + let matcher = make_glob_matcher(&buf).ok()?; + Some(matcher) } /// A segment of a dynamic builtin matcher. /// -/// Used internally by `BuiltinMatcher::Dynamic`'s lazy evaluation closure. +/// Used internally by `Lazy`'s lazy evaluation closure. #[derive(Clone, Debug)] enum MatcherSegment { Text(&'static str),