-
Notifications
You must be signed in to change notification settings - Fork 13
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
Split OutlineBuilder from GlyphBuilder #60
Conversation
f23f519
to
02b6da6
Compare
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 seems okay on a fairly quick pass. One concern I've just noticed now is that we're passing around the hashes of the identifiers, instead of passing the identifiers themselves; is there a specific rationale for this? It seems like it could possibly cause problems.
Inserting them into a set consumes them :D I figured we don't want that? And also avoid cloning. |
Okay, I think if we're worried about cloning the answer is to make sure we're using something like |
It does strike me as somewhat intrusive to make all identifiers |
Yea sorry! I didn't mean that we should use |
f769aff
to
6a8b05b
Compare
6a8b05b
to
e909958
Compare
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.
Cool, only real thing of note is that I think we can save ourselves needing to do all of these from_str
s, as mentioned in the more detailed comments.
Thanks again for pushing on with this!
80151f3
to
a9de5c4
Compare
This complicates the process slightly because now there's a hand-off between both builders, but is conceptually nicer. I think.