STAT table support

[khaledhosny]

Fixes #176.

Almost done, except for name id sharing (still trying to figure out how to reliably do that). Opening to see if CI is happy with what I’ve got so far.

[khaledhosny]

The LGTM alerts are in ANTLR generated code, is there away to silence these?

[josh-hadley]

The LGTM alerts are in ANTLR generated code, is there away to silence these?

There is a way to do it: https://lgtm.com/help/lgtm/customizing-file-classification

But, worth noting: the LGTM checks are informative only. They won't block the PR. So feel free to just ignore.

[cjchapman]

@khaledhosny I just made this change on develop which should help for future PRs:

afdko/.lgtm.yml

Lines 13 to 16 in 584fab7

	path_classifiers:
	generated:
	# Classify featgram.c as generated code so that it won't get alerts:
	- root/c/makeotf/makeotf_lib/source/hotconv/featgram.c

[khaledhosny]

But, worth noting: the LGTM checks are informative only. They won't block the PR. So feel free to just ignore.

It was a bit annoying since it kept adding new comments each time I push new changes.

@khaledhosny I just made this change on develop which should help for future PRs:

Thanks. I rebased my branch and it helps indeed.

[khaledhosny]

I’m trying to implement name ID sharing, but all ideas I can come up with seem unnecessarily complicated:

• It has to be deferred to table compilation time otherwise defining STAT before name will give different results.
• This means lots of changes to how the name table is built since the name ID will be known only when the table is about to be compiled not when the name is added, unlike the case now.
• The criteria of when to share and when not gives me headache, with all the possible combinations of platform, language, and encoding IDs.

Also the value of such sharing might not be that big:

• There is not much file saving, the actual strings are always shared even if used for different name IDs, so we are only saving the 12 bytes of the name record.
• There is already ElidedFallbackNameID, and other user name (e.g. for feature parameters don’t do such sharing).

Sorry for always coming back to this question 😄 but do we really want to to do this 😄 Last time to ask I promise.

[josh-hadley]

I’m trying to implement name ID sharing, but all ideas I can come up with seem unnecessarily complicated:
[...]
Sorry for always coming back to this question 😄 but do we really want to to do this 😄 Last time to ask I promise.

OK, let's proceed without nameRecord sharing for now and at least get the basic implementation integrated. We can defer the sharing feature until later or possibly devise a post-process action to handle it.

Thanks for your effort on this, @khaledhosny !

[khaledhosny]

All, good then. This should be more or less done, though I’d like to see #1128 before merging to check if the test coverage for the new code is good or not. The code is otherwise ready for review, and the documentation should follow once we are happy with the code.

[josh-hadley]

I’d like to see #1128 before merging to check if the test coverage for the new code is good or not.

I will try to work that out soon (via GHA, using a similar technique as I did in psautohint).

[cjchapman]

@khaledhosny Sorry, it looks like I made a mistake in the path to featgram.c in the .lgtm.yml where I indicated that it is a generated file. I think I've fixed it on develop in this commit.

[khaledhosny]

I think the code is ready for review and the test coverage is good.

[punchcutter]

@khaledhosny Did you update the spec as well? I don't see that update in here, but the tests reference the new sections of the spec.

[khaledhosny]

I was waiting until the code is finalized, but I guess it is time to do it.

[punchcutter]

Ok, no problem either way. Just wanted to double check.

[punchcutter]

Thanks Khaled! LGTM