-
Notifications
You must be signed in to change notification settings - Fork 168
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
[uforead.c] Revert glifRec array sort #1642
Conversation
c/shared/source/uforead/uforead.c
Outdated
@@ -1228,7 +1228,16 @@ static GLIF_Rec* findGLIFRecByName(ufoCtx h, char *glyphName) | |||
|
|||
GLIF_Rec* glif = bsearch(glifKey, h->data.glifRecs.array, h->data.glifRecs.cnt, sizeof(GLIF_Rec), glifRecNameComparator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if you're keeping this code around for future use but if we're reverting we should probably comment the bsearch and related lines out. (And I suppose if we do keep the lines we should probably keep the removed qsort in a comment as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry about this — I pushed this in a rush earlier as I was leaving for something & was looking over my PR again just now (should've get it in draft mode until I had re-reviewed). Needs some cleaning, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@skef Ah I added one more test right after your review — if that test looks good, I'll go ahead & merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Private secret just between us: I don't usually review test code unless I'm worried about uncovered cases, which is rare. If the code for a test is lower quality it will either pass when it shouldn't (unlikely) or someone will bring it to our attention when it fails.
Description
To fix #1641.
In #1575 in findGLIFRecByName function, I switched from using a for loop to using bsearch when finding a specific glifRec.
I had also added a qsort function since bsearch requires the array to be sorted, but I sorted by glyphName, which messed up the glyphOrder. We already were sorting a different way but sorting by glyphOrder instead.
Checklist: