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

[tx] Replace UFO content.plist/glyphList parsing with libxml2 #1543

Merged
merged 6 commits into from
Jul 21, 2022

Conversation

kaydeearts
Copy link
Collaborator

@kaydeearts kaydeearts commented Jul 20, 2022

Description

This PR replaces contents.plist (glyphList) parsing code with libxml2 #1521 as part of the tx parser replacement with libxml2 project

I added a comment below on your thoughts on something.

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

GLIF_Rec* glifRec;

glifRec = &h->data.glifRecs.array[index];

fileName = getKeyValue(h, "</string>", state);
if (fileName == NULL) {
fatal(h, ufoErrParse, "Encountered glyph reference in alternate layer's contents.plist with an empty file path. Text: '%s'.", getBufferContextPtr(h));
Copy link
Collaborator Author

@kaydeearts kaydeearts Jul 20, 2022

Choose a reason for hiding this comment

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

Not sure why it didn't highlight line 1280-1281, but that is what I meant to highlight here.

Curious to hear your thoughts about this — a key with an empty file path as its value will not fail anymore because the new libxml2 parsing code will simply skip a key that has a missing or NULL value, rather than fail like it did in the older parser code.

I think this is fine but wanted to hear if you have concerns about it. I didn't think of adding warnings since we're trying to reduce the amount of warning messages we have, but I can add them in this case if needed.

skef
skef previously approved these changes Jul 20, 2022
Copy link
Collaborator

@skef skef left a 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 I understand every bit of this PR but what I do understand looks good (relative to the one comment).

@@ -126,6 +126,10 @@ void ufoFree(ufoCtx h);
/* ufoFree() destroys the library context and all the resources allocated to
it. The temporary and debug data streams are closed. */

static void addGLIFRec(ufoCtx h, char* keyName, char* keyValue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are static they should be forward declarations in the .c file, not in the .h file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@skef thanks! I moved all the static functions to the c file, good to know!

@kaydeearts
Copy link
Collaborator Author

I'm not sure I understand every bit of this PR but what I do understand looks good (relative to the one comment).

If there's anything I could clarify for you, let me know!

Copy link
Collaborator

@skef skef left a comment

Choose a reason for hiding this comment

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

LGTM

@kaydeearts kaydeearts merged commit cd2db55 into develop Jul 21, 2022
@kaydeearts kaydeearts deleted the kd-libxml2-contentsplist branch July 21, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants