Skip to content

Compile MinMax BASE table statements in fea#1763

Merged
cmyr merged 6 commits into
mainfrom
base-minmax-statements
Nov 20, 2025
Merged

Compile MinMax BASE table statements in fea#1763
cmyr merged 6 commits into
mainfrom
base-minmax-statements

Conversation

@cmyr

@cmyr cmyr commented Nov 18, 2025

Copy link
Copy Markdown
Member

closes #1676

Comment thread fea-rs/src/token_tree/typed.rs Outdated
match self.iter().next().map(|t| t.kind()) {
Some(Kind::HorizAxisMinMaxKw) => true,
Some(Kind::VertAxisMinMaxKw) => false,
other => panic!("unexpected token in BaseTagList {other:?}"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

copy/pasta? the panic message mentions BaseTagList but we're in BaseMinMax

Comment thread fea-rs/src/compile/compile_ctx.rs Outdated
&mut self,
taglist: &typed::BaseTagList,
script_list: Option<typed::BaseScriptList>,
_minmax: impl Iterator<Item = typed::BaseMinMax>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is currently unused. potentially this method could check e.g. a script listed in MinMax exists in the script list, that the min/max values fit the signed i16 range, or duplicate entries etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thought about this a bit... we don't have access to the root script list here, so we can't check that (at least here) and the other bits of validation feel unnecessary, and likely cause us to deviate from fontmake in any case.

The stuff that it's important to validate here is stuff that we will later want to assume is true. (For instance, we will later want to find the index of a given baseline tag for items in the script list, so we need to ensure that those tags exist).

Comment on lines +112 to +116
rest.push(BaseLangSysRecord::new(*tag, table.compile()));
}
}

(dflt, rest)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BaseLangSysRecords should be sorted "in alphabetical order by BaseLangSysTag"

https://learn.microsoft.com/en-us/typography/opentype/spec/base

I think we aren't sorting them here (and write-fonts isn't either?) so if fonts have multiple language specific minmax entries for the same scripts we'd keep them in insertion order rather than alphabetical

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good catch!

@anthrotype anthrotype left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with comments

cmyr added 6 commits November 19, 2025 14:24
Or at least, the subset of this syntax that is also supported by feaLib.
- Better reuse of the logic that is identical for each axis
- invariants around ordering are now managed by the builder at
  construction time, instead of needing to be handled by the caller
@cmyr cmyr force-pushed the base-minmax-statements branch from 704ba5b to 98a67b1 Compare November 19, 2025 19:25
@cmyr cmyr added this pull request to the merge queue Nov 20, 2025
Merged via the queue into main with commit 443a2bf Nov 20, 2025
12 checks passed
@cmyr cmyr deleted the base-minmax-statements branch November 20, 2025 15:14
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.

Compile MinMax statements in BASE

2 participants