[tx] Replace UFO .glif file parsing with libxml2 #1556

kaydeearts · 2022-08-31T18:08:49Z

Description

This PR replaces UFO .glif file parsing code with libxml2 #1522 as part of the tx parser replacement with libxml2 project. It replaces quite a few parsing functions as well as adds some.

I'll be adding tests soon as part of this PR.

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

lgtm-com · 2022-08-31T18:49:33Z

This pull request introduces 4 alerts when merging 5aa50b1 into adb9656 - view on LGTM.com

new alerts:

3 for Missing return statement
1 for Wrong type of arguments to formatting function

kaydeearts · 2022-09-01T01:03:21Z

@josh-hadley @skef this is marked as a draft while I add more tests to the PR, but if you'd like to, you can start reviewing whenever available!

Note 1: I'll fix the lgtm alerts soon!

Note 2: I think there will be some refactoring needed in the future, perhaps not necessarily in this PR— I don't like that I had to keep adding global state tracking variables defined at the top of uforead. I would like to make this cleaner. I will still need to track which part of the UFO is being parsed, but I'll manage this better. Along with this, I'm planning on splitting the setFontDictKey function into a function for each part of the UFO, rather than have all of those keys parsed in one function. I may do all of this refactoring in a different PR, but am open to feedback on this anytime!

Thanks 😄

c/shared/source/uforead/uforead.c

josh-hadley

minor twiddles

c/shared/source/uforead/uforead.c

tests/makeotf_test.py

lgtm-com · 2022-09-24T00:20:04Z

This pull request introduces 2 alerts when merging efa4bb7 into f0e7023 - view on LGTM.com

new alerts:

2 for Declaration hides parameter

skef · 2022-09-29T04:37:06Z

c/shared/source/uforead/uforead.c

@@ -271,6 +282,14 @@ struct ufoCtx_ {
        ctlStreamCallbacks stm;
    } cb;
    dnaCtx dna;
+    struct {


This isn't a request for a change, but generally with these state structs you can rely on the struct name being part of the string. So no need to include "parsing" in the field name when the struct name is "parseState".

skef · 2022-09-29T05:30:53Z

c/shared/source/uforead/uforead.c

+    const char* filetype = "glyph";
+    int char_begin = 0;
+    int char_end = 0;
+    unsigned long *unicode = memNew(h, sizeof(unsigned long));


I didn't see a need for allocating this as opposed to declaring unicode as unsigned long and passing &unicode where needed. And that's certainly better than leaking the data, which I think is what is currently happening.

skef · 2022-09-29T05:35:02Z

c/shared/source/uforead/uforead.c

+    return cur;
+}
+
+static bool strEqual(char* string1, char* string2) {


Not declaring these string arguments as const char * will lead to many compiler warnings.

skef · 2022-09-29T05:35:33Z

c/shared/source/uforead/uforead.c

-    if ((!xmlStrEqual((cur)->name, (const xmlChar *) "dict"))) {
-        xmlFreeDoc(doc);
-        fatal(h, ufoErrSrcStream, "Error reading outermost <dict> in %s.\n", filename);
+    if (!strcmp("plist", filetype)) {


If we're doing strEqual() should we use it here too?

skef · 2022-09-29T05:41:57Z

c/shared/source/uforead/uforead.c

+            if (h->parseState.UFOFile == preParsingGLIF) {     /* when called from preParseGLIF */
+                if (strEqual(keyName, "com.adobe.type.cid.CID")) {
+                    keyValue = parseXMLKeyValue(h, cur);
+                    h->parseState.GLIFInfo.currentCID = atoi(keyValue);


Is it safe to pass NULL to atoi()? We advanced cur above so it could be NULL (?) and therefore the value passed back from parseXMLKeyValue() could be NULL. (Same question WRT next code section below.)

Good catch! I just created a test case for this and it crashes. Would you suggest I add the NULL check to right before setting h->parseState.GLIFInfo.currentCID, or should I return a value like -1 instead of NULL in parseXMLKeyValue so that it fixes this scenario for other callers?

I also realized that if the value of com.adobe.type.cid.CID is a blank value (like the example below) then com.adobe.type.cid.CID is set to 0. I feel like it also probably needs to remain -1 (which is currently what it starts with when unset).

<key>com.adobe.type.cid.CID</key> <integer></integer>

I'll look for more places where NULL may be returned from parseXMLKeyValue and the other functions. Thanks!

I think being more explicit with NULL checks is probably better for this. Returning "fake" values can eventually result in those values being used.

(Although I'd have to look in more detail to give you a conclusive answer ...)

skef · 2022-09-29T05:44:25Z

c/shared/source/uforead/uforead.c

+                    parseType1HintDataV2(h, cur->children);
+                }
+            }
+            cur = cur->next;


Could cur be NULL here because we advanced it once already?

lgtm-com · 2022-10-06T03:20:08Z

This pull request introduces 4 alerts when merging de56e35 into 3920d69 - view on LGTM.com

new alerts:

4 for Missing return statement

skef · 2022-10-06T03:18:34Z

c/shared/source/uforead/uforead.c

@@ -1682,32 +1681,39 @@ static char* getXmlAttrValue(xmlAttr *attr){
    return (char*) attr->children->content;
 }

-static void parseXMLLib(ufoCtx h, xmlNodePtr cur) {
+static bool setXMLLib(ufoCtx h, xmlNodePtr cur, char* keyName) {


You've declared this bool, presumably to track whether you ran into some problem parsing (given the check on the call), but you're not returning anything.

skef · 2022-10-06T03:24:29Z

c/shared/source/uforead/uforead.c

+    } else if (h->parseState.UFOFile == parsingGLIF) {  /* when called from parseGLIF */
+        if (strEqual(keyName, "com.adobe.type.autohint.v2")) {
+            h->parseState.type1HintDataV2 = true;
+            if (cur != NULL)


If cur is NULL, do we want to just ignore the fact there was a recognized key but no value here? Seems like that should be a warning at least.

For now, we've been aiming to reduce the amount of warnings until we add the verbosity flag option. However, I'm curious what @frankrolf thinks about this specifically. Frank, if within a UFO's .glif file's there is an empty com.adobe.type.autohint.v2, would it be helpful to the user to receive a warning?

My experimental repo branch (not yet copied back from my machine) has already pulled all the C-code-level logging messages into a single framework with multiple "levels". So after we start working with that code it will be easier to unify the logging across all of our tools.

Sounds good! So we can keep the warning messages for when we work with your changes

skef · 2022-10-06T03:31:47Z

c/shared/source/uforead/uforead.c

+static bool setType1HintDataV2(ufoCtx h, char* keyName, HintMask* curHintMask, xmlNodePtr cur) {
+    char* keyValue;
+    if (strEqual(keyName, "hintSetList")) {
+        parseHintSetListV2(h, cur);


Are all of the uses of cur in this method NULL-safe? Seems like since you parsed a key at the level above already, it should always have a value, it would be easier to check whether cur is NULL above, deal with that condition there, and always be sure cur is not NULL here.

kaydeearts · 2022-10-11T17:43:42Z

@skef @josh-hadley This PR is up to date with the latest changes you both requested, and I've added more tests covering edge cases & etc. Let me know if you have any more thoughts on this!

…parsing function

…eXMLComponent

…LGuideline

…struct

…istArrayValue

…ArrayValue, delete V1 parsing functions

…l2 parsing

…tDictKey & add sub-functions

…ll uforead fixes

…estigation

skef

At some point we should nail down what the return value of stm_xml_read represents, as it's sort of being used as a weird boolean right now, but I don't see that as a reason to hold things up.

kaydeearts force-pushed the kd-libxml2-glyphs branch from f451a7d to 0b425f0 Compare August 31, 2022 18:19

kaydeearts requested review from skef and josh-hadley September 1, 2022 00:58

skef reviewed Sep 6, 2022

View reviewed changes

josh-hadley requested changes Sep 6, 2022

View reviewed changes

c/shared/source/uforead/uforead.c Outdated Show resolved Hide resolved

tests/makeotf_test.py Show resolved Hide resolved

kaydeearts force-pushed the kd-libxml2-glyphs branch from ed50df7 to 23305ec Compare September 14, 2022 18:05

kaydeearts force-pushed the kd-libxml2-glyphs branch 6 times, most recently from 3b7fe6f to c895138 Compare September 29, 2022 01:16

kaydeearts marked this pull request as ready for review September 29, 2022 02:12

kaydeearts requested review from josh-hadley and skef September 29, 2022 02:13

skef reviewed Sep 29, 2022

View reviewed changes

kaydeearts force-pushed the kd-libxml2-glyphs branch from 6da6103 to 53d3aaa Compare October 6, 2022 02:50

kaydeearts requested a review from skef October 6, 2022 02:55

skef reviewed Oct 6, 2022

View reviewed changes

kaydeearts added 5 commits October 11, 2022 10:46

[uforead] Separate filetype-specific parsing out of general xml file …

87217e5

…parsing function

[uforead] Replace parseXMLAnchor

d0b0a32

[uforead] Replace parsePoint with libxml2, rename to parseXMLPoint

47dd026

[uforead] Replace parseComponent with libxml2 parsing, rename to pars…

e4d6615

…eXMLComponent

[uforead] Replace parseGuideline parsing w libxml2, rename to parseXM…

82a3f69

…LGuideline

kaydeearts added 21 commits October 11, 2022 10:46

[uforead] Need to reduce valueArray.cnt because strtok_r removes strings

7a68aea

[uforead] Define strtok_s as strtok_r for Windows + some cleanup

1b9f98a

[uforead] Replace string copy code with copyStr

db05db5

[uforead] Restructure parseState struct & add parsingGLIFInfo struct

abe3188

[uforead] Retrieve transform and glifRec from h->parseState.GLIFInfo …

ced4a00

…struct

[uforead] Replace parseFlexListV2 with libxml2 parsing, move setFlexL…

1ce43d2

…istArrayValue

[uforead] replace parseStem funcs with libxml2 parsing, move setStems…

e483ead

…ArrayValue, delete V1 parsing functions

[tx_test] Add test for hstem3, vstem3

0d7ef30

[uforead] Replace parseType1HintDataV2, parseHintSetListV2 with libxm…

ea8d981

…l2 parsing

[uforead] parse xmlGLIF keys within parseXMLGLIFKey instead of setFon…

02a9ec0

…tDictKey & add sub-functions

[uforead] Remove unnecessary params, function renaming + cleanup

8398fbc

[tx_test] Add glif parsing tests

5c29bde

[uforead] Use strtod instead of strtok_r, warning message cleanup

a9cbf13

[tx_test, uforead.c, tx_shared] Add more UFO tests for coverage + sma…

295c60a

…ll uforead fixes

[tx_test, uforead.c, tx_shared] Add more UFO tests for coverage + sma…

c2ae989

…ll uforead fixes

[tx_test] File renaming to have .pfa extensions instead of .subset

d59c1b5

[tx_data] Add more parsing tests

190fbc8

[uforead] Some parsing error fixes

d1d6cf7

[makeotf_test.py] Add back test with modified output for coverage inv…

12b25e6

…estigation

[tx_test] Add more test cases

142dad1

[uforead] Parsing fixes and edge case handling

4b14310

kaydeearts force-pushed the kd-libxml2-glyphs branch from b9f5b94 to 4b14310 Compare October 11, 2022 17:46

kaydeearts requested a review from skef October 11, 2022 17:47

josh-hadley approved these changes Oct 11, 2022

View reviewed changes

skef approved these changes Oct 11, 2022

View reviewed changes

kaydeearts merged commit 775c140 into develop Oct 12, 2022

kaydeearts deleted the kd-libxml2-glyphs branch October 12, 2022 00:32

This was referenced Oct 12, 2022

[tx] Replace UFO glif file parsing with libxml2 #1522

Closed

[uforead.h] Update uforead version #1566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tx] Replace UFO .glif file parsing with libxml2 #1556

[tx] Replace UFO .glif file parsing with libxml2 #1556

kaydeearts commented Aug 31, 2022 •

edited

Loading

lgtm-com bot commented Aug 31, 2022

kaydeearts commented Sep 1, 2022 •

edited

Loading

josh-hadley left a comment

lgtm-com bot commented Sep 24, 2022

skef Sep 29, 2022

skef Sep 29, 2022

skef Sep 29, 2022

skef Sep 29, 2022

skef Sep 29, 2022

kaydeearts Sep 30, 2022

skef Sep 30, 2022

skef Sep 30, 2022

skef Sep 29, 2022

lgtm-com bot commented Oct 6, 2022

skef Oct 6, 2022

skef Oct 6, 2022

kaydeearts Oct 11, 2022

skef Oct 11, 2022

kaydeearts Oct 11, 2022

skef Oct 6, 2022

kaydeearts commented Oct 11, 2022

skef left a comment

[tx] Replace UFO .glif file parsing with libxml2 #1556

[tx] Replace UFO .glif file parsing with libxml2 #1556

Conversation

kaydeearts commented Aug 31, 2022 • edited Loading

Description

Checklist:

lgtm-com bot commented Aug 31, 2022

kaydeearts commented Sep 1, 2022 • edited Loading

josh-hadley left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaydeearts commented Oct 11, 2022

skef left a comment

Choose a reason for hiding this comment

kaydeearts commented Aug 31, 2022 •

edited

Loading

kaydeearts commented Sep 1, 2022 •

edited

Loading