[fea-rs] Review feedback

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<PendingVariationIndex>) {
     fn resolve_metric(&mut self, metric: &typed::Metric) -> Metric {
         match metric {
             typed::Metric::Scalar(value) => value.parse_signed().into(),

fea-rs/src/compile/lookups.rs

@@ -53,7 +53,14 @@ pub(crate) use helpers::ClassDefBuilder2;
 // This exists because we use it to implement `LookupBuilder<T>`
 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;
 }