Skip to content
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

Housekeeping: let work tell us dependencies and what it reads/writes #359

Merged
merged 2 commits into from
Jul 7, 2023

Conversation

rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Jul 2, 2023

Large but mostly boring cleanup. Took longer than I care to admit to get everything wired up and working again :D

Move the knowledge of read/write access to the work. Define dependencies in terms of read access: if you can read it you depend on it and therefore must happen after it. Eliminate separate storage of dependencies, it was duplicative and quite weird when inconsistent with read_access.

Eliminate the macro for Context items, preferring to instead write reusable types ContextItem and ContextMap, as @cmyr once suggested.

Files (there are several) named orchestration.rs are probably the most interesting part; the rest is essentially repetitive updates to match the new interfaces. I would start with fontdrasil/src/orchestration.rs, then fontir/src/orchestration.rs and finally fontbe/src/orchestration.rs.

@rsheeter rsheeter changed the title [WIP] Let work tell us dependencies and read/write needs [WIP] Housekeeping: let work tell us dependencies and what it reads/writes Jul 3, 2023
@rsheeter rsheeter force-pushed the deps3 branch 5 times, most recently from c29184a to 877e01b Compare July 5, 2023 00:12
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.

Did a quick read through the various orchestration files, and have a few notes inline; overall this is a nice improvement, and it's nice to see you being more comfortable mucking about with the trait system.

fontdrasil/src/orchestration.rs Outdated Show resolved Hide resolved
fontir/src/orchestration.rs Outdated Show resolved Hide resolved
fontir/src/orchestration.rs Outdated Show resolved Hide resolved
fontir/src/orchestration.rs Show resolved Hide resolved
fontir/src/orchestration.rs Outdated Show resolved Hide resolved
fontir/src/orchestration.rs Outdated Show resolved Hide resolved
fontir/src/orchestration.rs Outdated Show resolved Hide resolved
fontir/src/serde.rs Show resolved Hide resolved
fontbe/src/orchestration.rs Show resolved Hide resolved
fontbe/src/orchestration.rs Outdated Show resolved Hide resolved
@rsheeter rsheeter changed the title [WIP] Housekeeping: let work tell us dependencies and what it reads/writes Housekeeping: let work tell us dependencies and what it reads/writes Jul 5, 2023
@rsheeter rsheeter marked this pull request as ready for review July 5, 2023 23:07
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.

A few more little non-blocking things, but overall this feels like a big improvement. I'm happy to see this get merged, and then we can iterate from here.

fontir/src/ir.rs Show resolved Hide resolved
fontdrasil/src/orchestration.rs Show resolved Hide resolved
fontir/src/orchestration.rs Show resolved Hide resolved
fontir/src/orchestration.rs Show resolved Hide resolved
@rsheeter rsheeter merged commit 376a425 into main Jul 7, 2023
6 checks passed
@rsheeter rsheeter deleted the deps3 branch July 7, 2023 04:23
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.

None yet

2 participants