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

[uforead] Set currentiFD to 0 #1541

Merged
merged 2 commits into from
Jul 18, 2022
Merged

Conversation

kaydeearts
Copy link
Collaborator

@kaydeearts kaydeearts commented Jul 18, 2022

Description

I found this while debugging #1538, which looks like the problem was mostly solved for by the libxml2 changes in #1515 and #1523. The only remaining issue seems to be the following:

currentiFD is a variable used to track which font dict index is pointed to in the FDArray. This is used in UFO parsing of fontinfo.plist, as well as glyphs files (in that order). While parsing glyphs files, the currentiFD is changed to -1. mergefonts parses fontinfo.plist again after the glyphs files, which causes a memory leak in non-M1 and a fail in M1 Macs because currentiFD should start with 0 (this points to the initial font dictionary).

Since the test doesn't fail on non-M1 unless ASAN is enabled, I did not add a new test.

This is a temporary fix as I'll rewrite glyphs parsing when working on #1522

I also included a small fix where the ufoErrSrcStream was being returned instead of ufoSuccess.

Checklist:

  • I have followed the Contribution Guidelines
  • I have added test code and data to prove that my code functions correctly
  • I have verified that new and existing tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Copy link
Collaborator

@josh-hadley josh-hadley left a comment

Choose a reason for hiding this comment

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

Looks good

@kaydeearts kaydeearts merged commit 625ce0e into develop Jul 18, 2022
@kaydeearts kaydeearts deleted the kd-mergefonts-m1-new-develop branch July 18, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants