From 39ac79a7f5afb812880e8f376b00a85c509c0cf5 Mon Sep 17 00:00:00 2001
From: Qunxin Liu <qxliu@google.com>
Date: Wed, 10 May 2023 10:22:49 -0700
Subject: [PATCH] address review comments

---
 src/hb-ot-stat-table.hh           | 20 +++----------
 src/hb-ot-var-fvar-table.hh       | 47 ++++++++++++++++---------------
 src/hb-subset-instancer-solver.hh |  3 ++
 src/hb-subset-plan.cc             |  3 --
 4 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/src/hb-ot-stat-table.hh b/src/hb-ot-stat-table.hh
index f9d9f6b24..8bf3ac090 100644
--- a/src/hb-ot-stat-table.hh
+++ b/src/hb-ot-stat-table.hh
@@ -64,10 +64,7 @@ static bool axis_value_is_outside_axis_range (hb_tag_t axis_tag, float axis_valu
     return false;
 
   Triple axis_range = user_axes_location->get (axis_tag);
-  if (axis_value < axis_range.minimum || axis_value > axis_range.maximum)
-    return true;
-
-  return false;
+  return (axis_value < axis_range.minimum || axis_value > axis_range.maximum);
 }
 
 struct StatAxisRecord
@@ -114,10 +111,7 @@ struct AxisValueFormat1
     hb_tag_t axis_tag = get_axis_tag (axis_records);
     float axis_value = get_value ();
 
-    if (axis_value_is_outside_axis_range (axis_tag, axis_value, user_axes_location))
-      return false;
-
-    return true;
+    return !axis_value_is_outside_axis_range (axis_tag, axis_value, user_axes_location);
   }
 
   bool subset (hb_subset_context_t *c,
@@ -172,10 +166,7 @@ struct AxisValueFormat2
     hb_tag_t axis_tag = get_axis_tag (axis_records);
     float axis_value = get_value ();
 
-    if (axis_value_is_outside_axis_range (axis_tag, axis_value, user_axes_location))
-      return false;
-
-    return true;
+    return !axis_value_is_outside_axis_range (axis_tag, axis_value, user_axes_location);
   }
 
   bool subset (hb_subset_context_t *c,
@@ -234,10 +225,7 @@ struct AxisValueFormat3
     hb_tag_t axis_tag = get_axis_tag (axis_records);
     float axis_value = get_value ();
 
-    if (axis_value_is_outside_axis_range (axis_tag, axis_value, user_axes_location))
-      return false;
-
-    return true;
+    return !axis_value_is_outside_axis_range (axis_tag, axis_value, user_axes_location);
   }
 
   bool subset (hb_subset_context_t *c,
diff --git a/src/hb-ot-var-fvar-table.hh b/src/hb-ot-var-fvar-table.hh
index 9abc57877..24b11ac67 100644
--- a/src/hb-ot-var-fvar-table.hh
+++ b/src/hb-ot-var-fvar-table.hh
@@ -39,6 +39,24 @@
 
 namespace OT {
 
+static bool axis_coord_pinned_or_within_axis_range (const hb_array_t<const F16DOT16> coords,
+                                                    unsigned axis_index,
+                                                    Triple axis_limit)
+{
+  float axis_coord = coords[axis_index].to_float ();
+  if (axis_limit.is_point ())
+  {
+    if (axis_limit.minimum != axis_coord)
+      return false;
+  }
+  else
+  {
+    if (axis_coord < axis_limit.minimum ||
+        axis_coord > axis_limit.maximum)
+      return false;
+  }
+  return true;
+}
 
 struct InstanceRecord
 {
@@ -62,18 +80,8 @@ struct InstanceRecord
         continue;
       
       Triple axis_limit = axes_location->get (*axis_tag);
-      float axis_coord = coords[i].to_float ();
-      if (axis_limit.is_point ())
-      {
-        if (axis_limit.minimum != axis_coord)
-          return false;
-      }
-      else
-      {
-        if (axis_coord < axis_limit.minimum ||
-            axis_coord > axis_limit.maximum)
-          return false;
-      }
+      if (!axis_coord_pinned_or_within_axis_range (coords, i, axis_limit))
+        return false;
     }
     return true;
   }
@@ -96,19 +104,12 @@ struct InstanceRecord
       if (axes_location->has (*axis_tag))
       {
         Triple axis_limit = axes_location->get (*axis_tag);
-        float axis_coord = coords[i].to_float ();
+        if (!axis_coord_pinned_or_within_axis_range (coords, i, axis_limit))
+          return_trace (false);
+        
+        //skip pinned axis
         if (axis_limit.is_point ())
-        {
-          if (axis_limit.minimum != axis_coord)
-            return_trace (false);
           continue;
-        }
-        else
-        {
-          if (axis_coord < axis_limit.minimum ||
-              axis_coord > axis_limit.maximum)
-            return_trace (false);
-        }
       }
 
       if (!c->serializer->embed (coords[i]))
diff --git a/src/hb-subset-instancer-solver.hh b/src/hb-subset-instancer-solver.hh
index 842ea73a9..911b37f4a 100644
--- a/src/hb-subset-instancer-solver.hh
+++ b/src/hb-subset-instancer-solver.hh
@@ -45,6 +45,9 @@ struct Triple {
   bool is_point () const
   { return minimum == middle && middle == maximum; }
 
+  bool contains (float point) const
+  { return minimum <= point && point <= maximum; }
+
   float minimum;
   float middle;
   float maximum;
diff --git a/src/hb-subset-plan.cc b/src/hb-subset-plan.cc
index ea17f8d08..ffb1671fa 100644
--- a/src/hb-subset-plan.cc
+++ b/src/hb-subset-plan.cc
@@ -181,9 +181,6 @@ static void _collect_layout_indices (hb_subset_plan_t     *plan,
       &conditionset_map
     };
     table.collect_feature_substitutes_with_variations (&c);
-    if (c.insert_catch_all_feature_variation_record)
-    {
-    }
   }
 #endif