Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Simplify Glyph #93

Merged
merged 4 commits into from
Mar 30, 2021
Merged

Simplify Glyph #93

merged 4 commits into from
Mar 30, 2021

Conversation

madig
Copy link
Collaborator

@madig madig commented Mar 27, 2021

Closes #92.

I'm keeping Outline around for only-once checking. Maybe... I can cut down the builders again and get the only-once logic back into the glif parser...

@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing 5940ed2 against 2233ab4

target old size new size difference
target/release/examples/open_ufo 1.75 MB 1.75 MB 856 Bytes (0.05%)
target/debug/examples/open_ufo 7.15 MB 7.15 MB -824 Bytes (-0.01%)

@madig madig changed the base branch from master to fix-lints March 27, 2021 16:58
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing df437e2 against ecd03ad

target old size new size difference
target/release/examples/open_ufo 1.75 MB 1.77 MB 20.34 KB (1.13%)
target/debug/examples/open_ufo 7.15 MB 7.15 MB -784 Bytes (-0.01%)

@madig
Copy link
Collaborator Author

madig commented Mar 28, 2021

I am now looking at de-Option-ing the libs of Anchor, Guideline, Contour, ContourPoint and Component. There, we have the added headache of identifiers. The replace_lib method takes care to assign an identifier.. but what if we don't have an Option to guide us? Auto-as# lib_mut even when nothing is added?

@cmyr
Copy link
Member

cmyr commented Mar 28, 2021

Is the option in these types a big problem, ergonomics-wise? I think that creating an identifier on access is reasonable, it's sort of like 'copy on write' semantics.

@madig
Copy link
Collaborator Author

madig commented Mar 28, 2021

Since there doesn't seem to be much code accessing those libs, I can't say, it is very slightly annoying when you have to, though. Maybe I'll leave it for now... I'll have to think about this more.

Base automatically changed from fix-lints to master March 28, 2021 21:56
@madig madig force-pushed the simplify-glyphs branch from 378c185 to 75cea06 Compare March 28, 2021 22:02
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing a7f1555 against 75ffac0

target old size new size difference
target/release/examples/open_ufo 1.75 MB 1.77 MB 19.21 KB (1.07%)
target/debug/examples/open_ufo 7.15 MB 7.15 MB -1.43 KB (-0.02%)

@madig
Copy link
Collaborator Author

madig commented Mar 28, 2021

I wonder why a release build is bigger now 🤔

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

One little note, otherwise this looks good to me!

src/glyph/builder.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing 12e2341 against 75ffac0

target old size new size difference
target/release/examples/open_ufo 1.75 MB 1.77 MB 19.2 KB (1.07%)
target/debug/examples/open_ufo 7.15 MB 7.15 MB -1.47 KB (-0.02%)

@madig madig force-pushed the simplify-glyphs branch from 24d2988 to 881b42f Compare March 29, 2021 22:02
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing ccc7770 against 75ffac0

target old size new size difference
target/release/examples/open_ufo 1.75 MB 1.77 MB 18.8 KB (1.05%)
target/debug/examples/open_ufo 7.15 MB 7.15 MB -2.22 KB (-0.03%)

@madig madig merged commit 9eb6576 into master Mar 30, 2021
@madig madig deleted the simplify-glyphs branch March 30, 2021 20:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Glyph: get Vecs out of Options?
2 participants