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

Shrink ProgressDrawTarget size #277

Merged
merged 2 commits into from
Apr 29, 2021
Merged

Shrink ProgressDrawTarget size #277

merged 2 commits into from
Apr 29, 2021

Conversation

mibac138
Copy link
Contributor

No description provided.

Only the amount of progress lines (total lines - orphan lines) was ever
used from `last_state`.

Size went down from 96 to 64 bytes
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

I like the last_state -> last_line_count change, that seems like a nice simplification. On the other hand, I'm less sure about removing the Option wrappers in favor of using sentinel values if the main benefit is just a few bytes of memory usage.

|| rate.is_none()
|| last_draw.is_none()
|| last_draw.unwrap().elapsed() > rate.unwrap()
|| rate == Duration::from_secs(0)
Copy link
Member

Choose a reason for hiding this comment

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

I honestly don't like this use of a sentinel value. The benefit of saving 8 bytes doesn't really seem worth it to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

|| rate == Duration::from_secs(0) could be removed without affecting behavior (leaving just forced || finished || elapsed > rate), and there is some precedent for this kind of usage of duration, see rust-lang/rust#73544, and rust-lang/rust#84084 which stabilizes Duration::is_zero. However, I'm fine with reverting this part of the PR if that's not convincing

@djc
Copy link
Member

djc commented Apr 29, 2021

Okay, I will merge this.

@djc djc merged commit 8f3773c into console-rs:main Apr 29, 2021
@mibac138 mibac138 deleted the shrink-pdt branch May 21, 2021 00:06
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