10000 Forbid empty glyph classes by simoncozens · Pull Request #2445 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Forbid empty glyph classes #2445

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Forbid empty glyph classes #2445

wants to merge 1 commit into from

Conversation

simoncozens
Copy link
Collaborator

makeotf forbids empty glyph classes. Given

feature test {
    sub [] a' by b;
} test;

makeotf says:

syntax error at "]" [foo.fea 2]

The feaLib parser allows them, but then chokes with an error later on:

  File "/usr/local/lib/python3.9/site-packages/fontTools/otlLib/builder.py", line 491, in buildFormat2Subtable
    ruleAsSubtable.Backtrack = [
  File "/usr/local/lib/python3.9/site-packages/fontTools/otlLib/builder.py", line 492, in <listcomp>
    st.BacktrackClassDef.classDefs[list(x)[0]]
IndexError: list index out of range

EXCEPT WHEN IT DOESN'T:

$ cat foo.fea
feature test {
    sub [a] by [];
} test;
$ fonttools feaLib -o stupid.otf foo.fea AGaramondPro-Regular.otf
$

While that's a cute way to delete a glyph, we now have sub ... by NULL so nobody should be relying on this unintended consequence, and we should follow the makeotf behaviour.

@simoncozens
Copy link
Collaborator Author
simoncozens commented Nov 10, 2021 •
edited
Loading

Oh, OK. So we actually have a test which not only ensures we can have empty glyph classes, but actively uses the empty glyph classes in pos rules. (baf4f56)

@simoncozens
Copy link
Collaborator Author

Come to think of it, we need to allow empty glyph classes in GDEF tables, so this is a non-starter. I'll instead forbid them at the point of use. But I'd still like to hear the rationale for allowing pos @something [].

@khaledhosny
Copy link
Collaborator

Already in 2019 I had no collection of ever writing this code or what it was for. I won’t object if you want to change it, if it breaks something we will eventually find out.

simoncozens added a commit to simoncozens/fonttools that referenced this pull request Nov 10, 2021
@simoncozens
See fonttools#2445
68e3e74
@simoncozens simoncozens mentioned this pull request Nov 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khaledhosny khaledhosny Awaiting requested review from khaledhosny

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simoncozens @khaledhosny
0