Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[subset] Fix namerecord ordering #2139

Merged
merged 1 commit into from
Feb 1, 2020

Conversation

qxliu76
Copy link
Collaborator

@qxliu76 qxliu76 commented Jan 29, 2020

(In theory original name records should be in sorted order already, I may need look into more details)
This will fix inconsistency with fontTools.
Also according to the spec, namerecords must be sorted
first by platform ID, then by platform-specific ID,
then by language ID, and then by name ID.

@behdad
Copy link
Member

behdad commented Jan 29, 2020

Can't we sort them in the output array?

@behdad
Copy link
Member

behdad commented Jan 29, 2020

Also don't use malloc. Use calloc.

@qxliu76
Copy link
Collaborator Author

qxliu76 commented Jan 29, 2020

Also don't use malloc. Use calloc.

done

This will fix inconsistency with fontTools.
Also according to the spec, namerecords must be sorted
first by platform ID, then by platform-specific ID,
then by language ID, and then by name ID.
@qxliu76
Copy link
Collaborator Author

qxliu76 commented Jan 29, 2020

Can't we sort them in the output array?

done

const NameRecord *b = (const NameRecord *)pb;

if (a->platformID != b->platformID)
return a->platformID < b->platformID ? -1 : +1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A slightly more compact way to do this is

return a->platformID - b->platformID;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also put the cmp in NameRecord itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A slightly more compact way to do this is
return a->platformID - b->platformID;

This isn't the common pattern on the table, will apply it later.

Also put the cmp in NameRecord itself.

Won't work as is, needs the following also, will provide it as a separate PR.

diff --git a/src/hb-array.hh b/src/hb-array.hh
index cbd6485a..2c1bb2fd 100644
--- a/src/hb-array.hh
+++ b/src/hb-array.hh
@@ -145,16 +145,22 @@ struct hb_array_t : hb_iter_with_fallback_t<hb_array_t<Type>, Type&>
     return not_found;
   }
 
-  hb_sorted_array_t<Type> qsort (int (*cmp_)(const void*, const void*))
+  HB_INTERNAL static int qsort_cmp (const void *pa, const void *pb)
+  {
+    Type *a = (Type *) pa;
+    Type *b = (Type *) pb;
+    return b->cmp (*a);
+  }
+  hb_sorted_array_t<Type> qsort ()
   {
     if (likely (length))
-      hb_qsort (arrayZ, length, this->get_item_size (), cmp_);
+      hb_qsort (arrayZ, length, this->get_item_size (), qsort_cmp);
     return hb_sorted_array_t<Type> (*this);
   }
-  hb_sorted_array_t<Type> qsort ()
+  hb_sorted_array_t<Type> qsort (int (*cmp_)(const void*, const void*))
   {
     if (likely (length))
-      hb_qsort (arrayZ, length, this->get_item_size (), Type::cmp);
+      hb_qsort (arrayZ, length, this->get_item_size (), cmp_);
     return hb_sorted_array_t<Type> (*this);
   }
   void qsort (unsigned int start, unsigned int end)
diff --git a/src/hb-ot-name-table.hh b/src/hb-ot-name-table.hh
index 0da56718..b3b1a9e3 100644
--- a/src/hb-ot-name-table.hh
+++ b/src/hb-ot-name-table.hh
@@ -112,30 +112,27 @@ struct NameRecord
   {
     unsigned int p = platformID;
     unsigned int e = encodingID;
-    
+
     return (p == 0 ||
             (p == 3 && (e == 0 || e == 1 || e == 10)));
   }
-  
-  static int cmp (const void *pa, const void *pb)
-  {
-    const NameRecord *a = (const NameRecord *)pa;
-    const NameRecord *b = (const NameRecord *)pb;
 
-    if (a->platformID != b->platformID)
-      return a->platformID < b->platformID ? -1 : +1;
+  int cmp (const NameRecord &record)
+  {
+    if (record.platformID != platformID)
+      return record.platformID < platformID ? -1 : +1;
 
-    if (a->encodingID != b->encodingID)
-      return a->encodingID < b->encodingID ? -1 : +1;
+    if (record.encodingID != encodingID)
+      return record.encodingID < encodingID ? -1 : +1;
 
-    if (a->languageID != b->languageID)
-      return a->languageID < b->languageID ? -1 : +1;
+    if (record.languageID != languageID)
+      return record.languageID < languageID ? -1 : +1;
 
-    if (a->nameID != b->nameID)
-      return a->nameID < b->nameID ? -1 : +1;
+    if (record.nameID != nameID)
+      return record.nameID < nameID ? -1 : +1;
 
-    if (a->length != b->length)
-      return a->length < b->length ? -1 : +1;
+    if (record.length != length)
+      return record.length < length ? -1 : +1;
 
     return 0;
   }

src/hb-ot-name-table.hh Show resolved Hide resolved
@ebraminio ebraminio merged commit 490ef1c into harfbuzz:master Feb 1, 2020
ebraminio added a commit that referenced this pull request Feb 1, 2020
Guess ternary was a bit more legible, apparently however we agreed to use subtraction,
#2139 (comment)
@qxliu76 qxliu76 deleted the name_table_fix branch February 4, 2020 21:28
@khaledhosny khaledhosny added the subset hb-subset related bugs label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subset hb-subset related bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants