-
Notifications
You must be signed in to change notification settings - Fork 24
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
Coverage table & class def fixups #188
Conversation
cmyr
commented
Dec 20, 2022
•
edited
Loading
edited
- There was a bug where we didn't dedupe entries in the coverage table before building
- There was an inefficiency where we would allow explicit class 0 entries when building classdefs
- I wanted a method on the write-fonts ClassDef to get the class for an item
This was a rather glaring oversight.
76f6354
to
b739871
Compare
/// | ||
/// Glyphs which have not been assigned a class are given class 0 | ||
pub fn get(&self, glyph: GlyphId) -> u16 { | ||
self.get_raw(glyph).unwrap_or(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap_or_default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally a personal thing, I just sometimes prefer the explicitness.
write-fonts/src/tables/layout.rs
Outdated
self.get_raw(glyph).unwrap_or(0) | ||
} | ||
|
||
// used for testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used by pub fn get so this is perhaps misleading?
.and_then(|idx| table.class_value_array.get(idx as usize)) | ||
.copied(), | ||
ClassDef::Format2(table) => table.class_range_records.iter().find_map(|rec| { | ||
(rec.start_glyph_id <= glyph && rec.end_glyph_id <= glyph).then_some(rec.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class range records are sorted by start glyph so bsearch? Or are you concerned the ones modified for write aren't? - if so plz comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they're sorted, but this collection can never be that huge, so I would expect the average cost of linear and binary search to come out about equal.
So mostly just "that'll do" 🤷
}), | ||
} | ||
} | ||
|
||
pub fn class_count(&self) -> u16 { | ||
//TODO: implement a good integer set!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wants to be an issue, presumably one that says "make me a memory safe hb_set"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let class = make_class([(4, 0), (5, 1)]); | ||
assert!(class.get_raw(GlyphId::new(4)).is_none()); | ||
assert_eq!(class.get_raw(GlyphId::new(5)), Some(1)); | ||
assert!(class.get_raw(GlyphId::new(100)).is_none()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not test the public get fn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is I actually want to know if there is a zero value in the record, instead of it being used as a placeholder; the public fn does unwrap_or(0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/nits
These are little things I wanted for fea-rs.
b739871
to
2430da1
Compare