Code review

fontbe/src/orchestration.rs

@@ -131,7 +131,7 @@ impl Context {
         ))
     }
 
-    pub fn read_only(&self) -> Context {
+    pub fn copy_read_only(&self) -> Context {
         self.copy(AccessControlList::read_only())
     }
 }
@@ -164,7 +164,7 @@ impl Context {
 
     pub fn get_features(&self) -> Arc<Vec<u8>> {
         let id = WorkId::Features;
-        self.acl.check_read_access(&id.clone().into());
+        self.acl.assert_read_access(&id.clone().into());
         {
             let rl = self.features.read();
             if rl.is_some() {
@@ -179,7 +179,7 @@ impl Context {
 
     pub fn set_features(&self, mut font: FontBuilder) {
         let id = WorkId::Features;
-        self.acl.check_write_access(&id.clone().into());
+        self.acl.assert_write_access(&id.clone().into());
         let font = font.build();
         self.maybe_persist(&self.paths.target_file(&id), &font);
         self.set_cached_features(font);

fontc/src/main.rs

@@ -19,7 +19,7 @@ use fontc::{
     Error,
 };
 use fontdrasil::{
-    orchestration::{access_none, access_one},
+    orchestration::{access_none, access_one, AccessFn},
     types::GlyphName,
 };
 use fontir::{
@@ -562,7 +562,7 @@ struct Job {
     // Things that must happen before we execute. Our  task can read these things.
     dependencies: HashSet<AnyWorkId>,
     // Things our job needs write access to
-    write_access: Arc<dyn Fn(&AnyWorkId) -> bool + Send + Sync>,
+    write_access: AccessFn<AnyWorkId>,
 }
 
 fn create_workload(change_detector: &mut ChangeDetector) -> Result<Workload, Error> {
@@ -783,8 +783,11 @@ mod tests {
             be_paths,
             &fe_root.read_only(),
         );
-        let mut result =
-            TestCompile::new(&change_detector, fe_root.read_only(), be_root.read_only());
+        let mut result = TestCompile::new(
+            &change_detector,
+            fe_root.read_only(),
+            be_root.copy_read_only(),
+        );
 
         let pre_success = workload.success.clone();
         while !workload.jobs_pending.is_empty() {

fontdrasil/src/orchestration.rs

@@ -4,6 +4,9 @@ use std::{collections::HashSet, fmt::Debug, hash::Hash, sync::Arc};
 
 pub const MISSING_DATA: &str = "Missing data, dependency management failed us?";
 
+/// A function that indicates whether access to something is permitted.
+pub type AccessFn<I> = Arc<dyn Fn(&I) -> bool + Send + Sync>;
+
 /// A unit of work safe to run in parallel
 ///
 /// Naively you'd think we'd just return FnOnce + Send but that didn't want to compile
@@ -23,7 +26,7 @@ where
     I: Eq + Hash + Debug,
 {
     // Returns true if you can write to the provided id
-    write_access: Arc<dyn Fn(&I) -> bool + Send + Sync>,
+    write_access: AccessFn<I>,
 
     // If present, what you can access through this context
     // Intent is root has None, task-specific Context only allows access to dependencies
@@ -49,18 +52,16 @@ impl<I: Eq + Hash + Debug> AccessControlList<I> {
     }
 }
 
-pub fn access_one<I: Debug + Eq + Send + Sync + 'static>(
-    id: I,
-) -> Arc<dyn Fn(&I) -> bool + Send + Sync> {
+pub fn access_one<I: Eq + Send + Sync + 'static>(id: I) -> AccessFn<I> {
     Arc::new(move |id2| id == *id2)
 }
 
-pub fn access_none<I: Eq + Send + Sync + 'static>() -> Arc<dyn Fn(&I) -> bool + Send + Sync> {
+pub fn access_none<I: Eq + Send + Sync + 'static>() -> AccessFn<I> {
     Arc::new(move |_| false)
 }
 
 impl<I: Eq + Hash + Debug> AccessControlList<I> {
-    pub fn check_read_access_to_any(&self, ids: &[I]) {
+    pub fn assert_read_access_to_any(&self, ids: &[I]) {
         if self.read_mask.is_none() {
             return;
         }
@@ -71,7 +72,7 @@ impl<I: Eq + Hash + Debug> AccessControlList<I> {
         }
     }
 
-    pub fn check_read_access(&self, id: &I) {
+    pub fn assert_read_access(&self, id: &I) {
         if !self
             .read_mask
             .as_ref()
@@ -82,13 +83,13 @@ impl<I: Eq + Hash + Debug> AccessControlList<I> {
         }
     }
 
-    pub fn check_write_access_to_any(&self, ids: &[I]) {
+    pub fn assert_write_access_to_any(&self, ids: &[I]) {
         if !ids.iter().any(|id| (self.write_access)(id)) {
             panic!("Illegal access to {ids:?}");
         }
     }
 
-    pub fn check_write_access(&self, id: &I) {
+    pub fn assert_write_access(&self, id: &I) {
         if !(self.write_access)(id) {
             panic!("Illegal access to {id:?}");
         }

fontir/src/glyph.rs

@@ -38,7 +38,7 @@ fn has_components_and_contours(glyph: &Glyph) -> bool {
         .any(|inst| !inst.components.is_empty() && !inst.contours.is_empty())
 }
 
-/// Does the Glyph uses components whose transform is different at different locations designspace?
+/// Does the Glyph use the same set of components, including transform, for all instances?
 fn has_consistent_component_transforms(glyph: &Glyph) -> bool {
     distinct_component_seqs(glyph).len() <= 1
 }
@@ -55,6 +55,9 @@ fn name_for_derivative(base_name: &GlyphName, names_in_use: &IndexSet<GlyphName>
     }
 }
 
+/// Returns a tuple of (simple glyph, composite glyph).
+///
+/// The former contains all the contours, the latter contains all the components.
 fn split_glyph(glyph_order: &IndexSet<GlyphName>, original: &Glyph) -> (Glyph, Glyph) {
     // Make a simple glyph by erasing the components from it
     let mut simple_glyph = original.clone();

fontir/src/orchestration.rs

@@ -155,7 +155,7 @@ impl Context {
 
     pub fn get_static_metadata(&self) -> Arc<ir::StaticMetadata> {
         let ids = [WorkId::InitStaticMetadata, WorkId::FinalizeStaticMetadata];
-        self.acl.check_read_access_to_any(&ids);
+        self.acl.assert_read_access_to_any(&ids);
         {
             let rl = self.static_metadata.read();
             if rl.is_some() {
@@ -169,21 +169,21 @@ impl Context {
 
     pub fn set_static_metadata(&self, ir: ir::StaticMetadata) {
         let ids = [WorkId::InitStaticMetadata, WorkId::FinalizeStaticMetadata];
-        self.acl.check_write_access_to_any(&ids);
+        self.acl.assert_write_access_to_any(&ids);
         self.maybe_persist(&self.paths.target_file(&ids[0]), &ir);
         self.set_cached_static_metadata(ir);
     }
 
     pub fn get_glyph_ir(&self, glyph_name: &GlyphName) -> Arc<ir::Glyph> {
         let id = WorkId::Glyph(glyph_name.clone());
-        self.acl.check_read_access(&id);
+        self.acl.assert_read_access(&id);
         let rl = self.glyph_ir.read();
         rl.get(glyph_name).expect(MISSING_DATA).clone()
     }
 
     pub fn set_glyph_ir(&self, ir: ir::Glyph) {
         let id = WorkId::Glyph(ir.name.clone());
-        self.acl.check_write_access(&id);
+        self.acl.assert_write_access(&id);
         self.maybe_persist(&self.paths.target_file(&id), &ir);
         let mut wl = self.glyph_ir.write();
         wl.insert(ir.name.clone(), Arc::from(ir));
@@ -196,7 +196,7 @@ impl Context {
 
     pub fn get_features(&self) -> Arc<ir::Features> {
         let id = WorkId::Features;
-        self.acl.check_read_access(&id);
+        self.acl.assert_read_access(&id);
         {
             let rl = self.feature_ir.read();
             if rl.is_some() {
@@ -210,7 +210,7 @@ impl Context {
 
     pub fn set_features(&self, ir: ir::Features) {
         let id = WorkId::Features;
-        self.acl.check_write_access(&id);
+        self.acl.assert_write_access(&id);
         self.maybe_persist(&self.paths.target_file(&id), &ir);
         self.set_cached_features(ir);
     }