[glyph closure] address review comments

Author: qxliu76

read-fonts/src/tables/gsub/closure.rs

@@ -20,7 +20,7 @@ use super::{
 #[cfg(feature = "std")]
 use crate::tables::layout::{
     ContextFormat1, ContextFormat2, ContextFormat3, Intersect, LayoutLookupList, LookupClosure,
-    LookupClosureCtx,
+    LookupClosureCtx, SeqCache,
 };
 
 // we put ClosureCtx in its own module to enforce visibility rules;
@@ -133,12 +133,7 @@ mod ctx {
 
         // Return true if we have visited this lookup with current set of glyphs
         pub(super) fn is_lookup_done(&mut self, lookup_index: u16) -> bool {
-            let cur_active_glyphs = if let Some(cur_active_glyphs) = self.active_glyphs_stack.last()
-            {
-                cur_active_glyphs
-            } else {
-                &*self.glyphs
-            };
+            let cur_active_glyphs = self.active_glyphs_stack.last().unwrap_or(self.glyphs);
 
             let (count, covered) = self
                 .done_lookups_glyphs
@@ -888,10 +883,8 @@ impl GlyphClosure for ContextFormat2<'_> {
         let lookups = lookup_list.lookups();
         let mut seen_sequence_indices = IntSet::new();
 
-        let mut input_cache = FnvHashMap::default();
-        let mut backtrack_cache = FnvHashMap::default();
-        let mut lookahead_cache = FnvHashMap::default();
         let mut intersected_class_cache = FnvHashMap::default();
+        let mut seq_cache = SeqCache::default();
         for (i, rule_set) in self
             .rule_sets()
             .enumerate()
@@ -916,9 +909,7 @@ impl GlyphClosure for ContextFormat2<'_> {
                     &input_class_def,
                     backtrack_class_def.as_ref(),
                     lookahead_class_def.as_ref(),
-                    &mut input_cache,
-                    &mut backtrack_cache,
-                    &mut lookahead_cache,
+                    &mut seq_cache,
                 ) {
                     continue;
                 }

read-fonts/src/tables/layout.rs

@@ -20,7 +20,7 @@ use crate::collections::IntSet;
 #[cfg(feature = "std")]
 pub(crate) use closure::{
     ContextFormat1, ContextFormat2, ContextFormat3, LayoutLookupList, LookupClosure,
-    LookupClosureCtx, MAX_LOOKUP_VISIT_COUNT, MAX_NESTING_LEVEL,
+    LookupClosureCtx, SeqCache, MAX_LOOKUP_VISIT_COUNT, MAX_NESTING_LEVEL,
 };
 
 #[cfg(feature = "std")]

read-fonts/src/tables/layout/closure.rs

@@ -631,6 +631,13 @@ pub(crate) enum Format2Rule<'a> {
     Chain(ChainedClassSequenceRule<'a>),
 }
 
+#[derive(Default)]
+pub(crate) struct SeqCache {
+    input: FnvHashMap<u16, bool>,
+    backtrack: FnvHashMap<u16, bool>,
+    lookahead: FnvHashMap<u16, bool>,
+}
+
 impl ContextFormat2<'_> {
     pub(crate) fn coverage(&self) -> Option<Result<CoverageTable<'_>, ReadError>> {
         match self {
@@ -735,20 +742,16 @@ impl Format2Rule<'_> {
         input_class_def: &ClassDef,
         backtrack_class_def: Option<&ClassDef>,
         lookahead_class_def: Option<&ClassDef>,
-        input_cache: &mut FnvHashMap<u16, bool>,
-        backtrack_cache: &mut FnvHashMap<u16, bool>,
-        lookahead_cache: &mut FnvHashMap<u16, bool>,
+        seq_cache: &mut SeqCache,
     ) -> bool {
         match self {
-            Self::Plain(table) => table.intersects(glyphs, input_class_def, input_cache),
+            Self::Plain(table) => table.intersects(glyphs, input_class_def, &mut seq_cache.input),
             Self::Chain(table) => table.intersects(
                 glyphs,
                 input_class_def,
                 backtrack_class_def,
                 lookahead_class_def,
-                input_cache,
-                backtrack_cache,
-                lookahead_cache,
+                seq_cache,
             ),
         }
     }
@@ -760,13 +763,9 @@ fn intersects_class(
     class: u16,
     cache: &mut FnvHashMap<u16, bool>,
 ) -> bool {
-    if let Some(v) = cache.get(&class) {
-        return *v;
-    }
-
-    let v = class_def.intersects_class_glyphs(glyphs, class);
-    cache.insert(class, v);
-    v
+    *cache
+        .entry(class)
+        .or_insert_with(|| class_def.intersects_class_glyphs(glyphs, class))
 }
 impl ClassSequenceRule<'_> {
     fn intersects(
@@ -789,36 +788,40 @@ impl ChainedClassSequenceRule<'_> {
         input_class_def: &ClassDef,
         backtrack_class_def: Option<&ClassDef>,
         lookahead_class_def: Option<&ClassDef>,
-        input_cache: &mut FnvHashMap<u16, bool>,
-        backtrack_cache: &mut FnvHashMap<u16, bool>,
-        lookahead_cache: &mut FnvHashMap<u16, bool>,
+        seq_cache: &mut SeqCache,
     ) -> bool {
         if !self
             .input_sequence()
             .iter()
-            .all(|c| intersects_class(input_class_def, glyphs, c.get(), input_cache))
+            .all(|c| intersects_class(input_class_def, glyphs, c.get(), &mut seq_cache.input))
         {
             return false;
         }
 
         if let Some(backtrack_class_def) = backtrack_class_def {
-            if !self
-                .backtrack_sequence()
-                .iter()
-                .all(|c| intersects_class(backtrack_class_def, glyphs, c.get(), backtrack_cache))
-            {
+            if !self.backtrack_sequence().iter().all(|c| {
+                intersects_class(
+                    backtrack_class_def,
+                    glyphs,
+                    c.get(),
+                    &mut seq_cache.backtrack,
+                )
+            }) {
                 return false;
             }
         } else if self.backtrack_glyph_count() != 0 {
             return false;
         }
 
         if let Some(lookahead_class_def) = lookahead_class_def {
-            if !self
-                .lookahead_sequence()
-                .iter()
-                .all(|c| intersects_class(lookahead_class_def, glyphs, c.get(), lookahead_cache))
-            {
+            if !self.lookahead_sequence().iter().all(|c| {
+                intersects_class(
+                    lookahead_class_def,
+                    glyphs,
+                    c.get(),
+                    &mut seq_cache.lookahead,
+                )
+            }) {
                 return false;
             }
         } else if self.lookahead_glyph_count() != 0 {
@@ -961,9 +964,7 @@ impl Intersect for ContextFormat2<'_> {
             }
         };
 
-        let mut input_cache = FnvHashMap::default();
-        let mut backtrack_cache = FnvHashMap::default();
-        let mut lookahead_cache = FnvHashMap::default();
+        let mut seq_cache = SeqCache::default();
         for rule_set in self.rule_sets().enumerate().filter_map(|(c, rule_set)| {
             input_class_def
                 .intersects_class_glyphs(&retained_coverage_glyphs, c as u16)
@@ -979,9 +980,7 @@ impl Intersect for ContextFormat2<'_> {
                     &input_class_def,
                     backtrack_class_def.as_ref(),
                     lookahead_class_def.as_ref(),
-                    &mut input_cache,
-                    &mut backtrack_cache,
-                    &mut lookahead_cache,
+                    &mut seq_cache,
                 ) {
                     return Ok(true);
                 }
@@ -1025,9 +1024,7 @@ impl LookupClosure for ContextFormat2<'_> {
             }
         };
 
-        let mut input_cache = FnvHashMap::default();
-        let mut backtrack_cache = FnvHashMap::default();
-        let mut lookahead_cache = FnvHashMap::default();
+        let mut seq_cache = SeqCache::default();
         for rule_set in self.rule_sets().enumerate().filter_map(|(c, rule_set)| {
             input_class_def
                 .intersects_class_glyphs(&retained_coverage_glyphs, c as u16)
@@ -1051,9 +1048,7 @@ impl LookupClosure for ContextFormat2<'_> {
                     &input_class_def,
                     backtrack_class_def.as_ref(),
                     lookahead_class_def.as_ref(),
-                    &mut input_cache,
-                    &mut backtrack_cache,
-                    &mut lookahead_cache,
+                    &mut seq_cache,
                 ) {
                     continue;
                 }