From afa1882ddb07e1140650210d82bd7755172f53b2 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 7 Sep 2023 13:22:17 -0400 Subject: [PATCH] [write-fonts] Improve validation API This significantly reduces indent drift when dealing with arrays; where previously you needed to indent 1) to start an array, 2) to manually loop and 3) to declare each item, we now have a single method that lets you pass in an iterator and a closure to be called on each item, and we handle the rest. The diff shows that this is significantly less painful. --- write-fonts/src/tables/gpos.rs | 46 +++++++------------ write-fonts/src/validate.rs | 81 ++++++++++++++++++++++------------ 2 files changed, 69 insertions(+), 58 deletions(-) diff --git a/write-fonts/src/tables/gpos.rs b/write-fonts/src/tables/gpos.rs index 715160d91..f3e045e97 100644 --- a/write-fonts/src/tables/gpos.rs +++ b/write-fonts/src/tables/gpos.rs @@ -111,21 +111,14 @@ impl PairPosFormat1 { fn check_format_consistency(&self, ctx: &mut ValidationCtx) { let vf1 = self.compute_value_format1(); let vf2 = self.compute_value_format2(); - ctx.in_array(|ctx| { - for pairset in &self.pair_sets { - ctx.array_item(|ctx| { - ctx.in_field("pair_value_records", |ctx| { - ctx.in_array(|ctx| { - if pairset.pair_value_records.iter().any(|pairset| { - pairset.value_record1.format() != vf1 - || pairset.value_record2.format() != vf2 - }) { - ctx.report("all ValueRecords must have same format") - } - }) - }) - }) - } + ctx.with_array_items(self.pair_sets.iter(), |ctx, item| { + ctx.in_field("pair_value_records", |ctx| { + if item.pair_value_records.iter().any(|pairset| { + pairset.value_record1.format() != vf1 || pairset.value_record2.format() != vf2 + }) { + ctx.report("all ValueRecords must have same format") + } + }) }) } } @@ -164,21 +157,14 @@ impl PairPosFormat2 { ctx.report("class1_records length must match number of class1 classes"); } ctx.in_field("class1_records", |ctx| { - ctx.in_array(|ctx| { - for record in &self.class1_records { - ctx.array_item(|ctx| { - if record.class2_records.len() != n_class_2s as usize { - ctx.report( - "class2_records length must match number of class2 classes ", - ); - } - if record.class2_records.iter().any(|rec| { - rec.value_record1.format() != format_1 - || rec.value_record2.format() != format_2 - }) { - ctx.report("all value records should report the same format"); - } - }) + ctx.with_array_items(self.class1_records.iter(), |ctx, c1rec| { + if c1rec.class2_records.len() != n_class_2s as usize { + ctx.report("class2_records length must match number of class2 classes "); + } + if c1rec.class2_records.iter().any(|rec| { + rec.value_record1.format() != format_1 || rec.value_record2.format() != format_2 + }) { + ctx.report("all value records should report the same format"); } }) }); diff --git a/write-fonts/src/validate.rs b/write-fonts/src/validate.rs index 60fb248a0..af1580a06 100644 --- a/write-fonts/src/validate.rs +++ b/write-fonts/src/validate.rs @@ -105,23 +105,17 @@ impl ValidationCtx { self.with_elem(LocationElem::Field(name), f); } - /// Run the provided closer in the context of an array. - pub fn in_array(&mut self, f: impl FnOnce(&mut ValidationCtx)) { - self.with_elem(LocationElem::Index(0), f); - } - - /// Run the provided closer in the context of a new array item. + /// Run the provided closer for each item in an array. /// - /// This must only be called in a closure passed to [in_array][Self::in_array]. - pub fn array_item(&mut self, f: impl FnOnce(&mut ValidationCtx)) { - assert!(matches!( - self.cur_location.last(), - Some(LocationElem::Index(_)) - )); - f(self); - match self.cur_location.last_mut() { - Some(LocationElem::Index(i)) => *i += 1, - _ => panic!("array_item called outside of array"), + /// This handles tracking the active item, so that validation errors can + /// be associated with the correct index. + pub fn with_array_items<'a, T: 'a>( + &mut self, + iter: impl Iterator, + mut f: impl FnMut(&mut ValidationCtx, &T), + ) { + for (i, item) in iter.enumerate() { + self.with_elem(LocationElem::Index(i), |ctx| f(ctx, item)) } } @@ -166,6 +160,9 @@ impl Display for ValidationError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, "\"{}\"", self.error)?; let mut indent = 0; + if self.location.len() < 2 { + return f.write_str("no parent location available"); + } for (i, window) in self.location.windows(2).enumerate() { let prev = &window[0]; @@ -194,13 +191,7 @@ impl Display for ValidationError { impl Validate for Vec { fn validate_impl(&self, ctx: &mut ValidationCtx) { - ctx.in_array(|ctx| { - for item in self.iter() { - ctx.array_item(|ctx| { - item.validate_impl(ctx); - }) - } - }); + ctx.with_array_items(self.iter(), |ctx, item| item.validate_impl(ctx)) } } @@ -227,12 +218,46 @@ impl Validate for Option { impl Validate for BTreeSet { fn validate_impl(&self, ctx: &mut ValidationCtx) { - ctx.in_array(|ctx| { - for item in self.iter() { - ctx.array_item(|ctx| { - item.validate_impl(ctx); + ctx.with_array_items(self.iter(), |ctx, item| item.validate_impl(ctx)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn sanity_check_array_validation() { + #[derive(Clone, Debug, Copy)] + struct Derp(i16); + + struct DerpStore { + derps: Vec, + } + + impl Validate for Derp { + fn validate_impl(&self, ctx: &mut ValidationCtx) { + if self.0 > 7 { + ctx.report("this derp is too big!!"); + } + } + } + + impl Validate for DerpStore { + fn validate_impl(&self, ctx: &mut ValidationCtx) { + ctx.in_table("DerpStore", |ctx| { + ctx.in_field("derps", |ctx| self.derps.validate_impl(ctx)) }) } - }); + } + + let my_derps = DerpStore { + derps: [1i16, 0, 3, 4, 12, 7, 6].into_iter().map(Derp).collect(), + }; + + let report = my_derps.validate().err().unwrap(); + assert_eq!(report.errors.len(), 1); + // ensure that the index is being calculated correctly + assert!(report.to_string().contains(".derps[4]")); } }