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

fn rav1d_worker_task: Refactor state machine #671

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

rinon
Copy link
Collaborator

@rinon rinon commented Jan 10, 2024

rav1d_worker_task involves complex control flow with gotos, and therefore was transpiled into a state machine. This refactoring restores the original structure with minimal additional structured control flow to emulate gotos.

@rinon rinon requested a review from kkysen January 10, 2024 20:33
@kkysen kkysen changed the title Refactor rav1d_worker_task fn rav1d_worker_task: Refactor state machine Jan 10, 2024
@rinon rinon force-pushed the sjc/rav1d_worker_task-cleanup branch 2 times, most recently from 7509587 to ef6b864 Compare January 12, 2024 01:36
src/thread_task.rs Outdated Show resolved Hide resolved
src/thread_task.rs Outdated Show resolved Hide resolved
src/thread_task.rs Outdated Show resolved Hide resolved
src/thread_task.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

I think https://github.com/memorysafety/rav1d/pull/671/files#r1449731224 is the only thing that's wrong, though I made some other suggestions that would IMO greatly help readability.

@rinon rinon force-pushed the sjc/rav1d_worker_task-cleanup branch from ef6b864 to 4b3f2a2 Compare January 12, 2024 21:09
@rinon rinon requested a review from kkysen January 12, 2024 21:10
@rinon
Copy link
Collaborator Author

rinon commented Jan 12, 2024

Thanks for the useful feedback, I've applied everything except for the labeled block for found, just want to avoid extra indentation if we can. I forgot that labelled blocks were added, but that cleans up the goto next code perfectly, thanks.

Copy link
Collaborator

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

There's that one // next: comment I think you should add back, but otherwise everything LGTM now. I slightly prefer actual labeled blocks vs. a // label: comment, but the indentation isn't great either, so it's fine as is.

src/thread_task.rs Show resolved Hide resolved
`rav1d_worker_task` involves complex control flow with gotos, and therefore was transpiled into a state machine. This refactoring restores the original structure with minimal additional structured control flow to emulate gotos.
@rinon rinon force-pushed the sjc/rav1d_worker_task-cleanup branch from 4b3f2a2 to d28be39 Compare January 17, 2024 21:09
@rinon rinon merged commit d1867f6 into main Jan 17, 2024
34 checks passed
@rinon rinon deleted the sjc/rav1d_worker_task-cleanup branch January 17, 2024 21:32
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.

2 participants