From 06638826c22362f12b3764ed11421735151fa43b Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Tue, 28 Nov 2023 12:35:57 -0500 Subject: [PATCH] [fea-rs] Review feedback --- fea-rs/src/compile/compile_ctx.rs | 15 ++++++--------- fea-rs/src/compile/lookups.rs | 9 ++++++++- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/fea-rs/src/compile/compile_ctx.rs b/fea-rs/src/compile/compile_ctx.rs index 847fbed8f..2701e105d 100644 --- a/fea-rs/src/compile/compile_ctx.rs +++ b/fea-rs/src/compile/compile_ctx.rs @@ -1094,15 +1094,13 @@ impl<'a> CompilationCtx<'a> { if let Some(adv) = record.advance() { let adv = self.resolve_metric(&adv); if self.vertical_feature.in_eligible_vertical_feature() { - return ValueRecord { - y_advance: Some(adv), - ..Default::default() - }; + return ValueRecord::new() + .with_y_advance(adv.default) + .with_y_advance_device(adv.device_or_deltas); } else { - return ValueRecord { - x_advance: Some(adv), - ..Default::default() - }; + return ValueRecord::new() + .with_x_advance(adv.default) + .with_x_advance_device(adv.device_or_deltas); } } @@ -1141,7 +1139,6 @@ impl<'a> CompilationCtx<'a> { result } - //fn resolve_metric(&mut self, metric: &typed::Metric) -> (i16, Option) { fn resolve_metric(&mut self, metric: &typed::Metric) -> Metric { match metric { typed::Metric::Scalar(value) => value.parse_signed().into(), diff --git a/fea-rs/src/compile/lookups.rs b/fea-rs/src/compile/lookups.rs index 4e6c6d356..3556544c2 100644 --- a/fea-rs/src/compile/lookups.rs +++ b/fea-rs/src/compile/lookups.rs @@ -53,7 +53,14 @@ pub(crate) use helpers::ClassDefBuilder2; // This exists because we use it to implement `LookupBuilder` pub(crate) trait Builder { type Output; - // the var_store is only used in GPOS, but we pass it everywhere + // the var_store is only used in GPOS, but we pass it everywhere. + // This is annoying but feels like the lesser of two evils. It's easy to + // ignore this argument where it isn't used, and this makes the logic + // in LookupBuilder simpler, since it is identical for GPOS/GSUB. + // + // It would be nice if this could then be Option<&mut T>, but that type is + // annoying to work with, as Option<&mut _> doesn't impl Copy, so you need + // to do a dance anytime you use it. fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output; }