Skip to content

std.Progress: fix suffix printing #10448

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

Merged
merged 2 commits into from
Feb 14, 2022
Merged

Conversation

erikarvstedt
Copy link
Contributor

@erikarvstedt erikarvstedt commented Dec 29, 2021

Copy of main commit msg

Previously, suffix was copied to output_buffer at position max_end, thereby writing into reserved space after max_end.
This only worked because suffix was not larger than bytes_needed_for_esc_codes_at_end (otherwise there'd be a potential buffer overrun) and no escape codes at end are actually written.

Since 2d5b2bf, escape codes are no longer written to the end of the buffer. They are now written exclusively to the front of the buffer.
This allows removing bytes_needed_for_esc_codes_at_end and simplifying the suffix printing logic.

This also fixes the bug that the ellipse suffix was not printed in Windows terminals because end.* > max_end was never true.

This helps debugging the test.
@erikarvstedt erikarvstedt force-pushed the fix-progress branch 3 times, most recently from 51c9369 to 8d13e66 Compare December 29, 2021 23:37
@erikarvstedt erikarvstedt changed the title std.Progress: fix buffer logic bug std.Progress: fix suffix printing Dec 29, 2021
Previously, `suffix` was copied to `output_buffer` at position
`max_end`, thereby writing into reserved space after `max_end`.
This only worked because `suffix` was not larger than
`bytes_needed_for_esc_codes_at_end` (otherwise there'd be a potential
buffer overrun) and no escape codes at end are actually written.

Since 2d5b2bf, escape codes are no
longer written to the end of the buffer. They are now written
exclusively to the front of the buffer.
This allows removing `bytes_needed_for_esc_codes_at_end` and
simplifying the suffix printing logic.

This also fixes the bug that the ellipse suffix was not printed in
Windows terminals because `end.* > max_end` was never true.
@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Dec 30, 2021
@erikarvstedt
Copy link
Contributor Author

@SebastianKeller, you recently commited changes to Progress.zig. Can you have a look at this?
The CI failure is only due to a build timeout on Windows.

@SebastianKeller
Copy link

Hi @erikarvstedt

isn't that ci run old? The other ci runs ran end of december 2021, the failing ziglang.zig run is not available anymore. These runs did not include my changes. Also there's a pinned issue about failing ci.

Maybe I'm missing something, but I don't think this is related to my changes.

@erikarvstedt
Copy link
Contributor Author

erikarvstedt commented Feb 13, 2022

Sorry, @SebastianKeller, my previous message wasn't very clear.
Your changes are perfectly fine, I'm looking for a reviewer for this PR and you seemed like the right person to ask.
(I referred to the CI failure to point out that it's not caused by this PR).

@erikarvstedt
Copy link
Contributor Author

@SebastianKeller, thanks!
Sorry, another misunderstanding: I thought you had commit rights to this repo, but in fact your PR was merged by vexu.

speed_factor allows slowing down the test to ease manual debugging of the progress output.

@erikarvstedt
Copy link
Contributor Author

@Vexu, could you review this bugfix PR?

@Vexu Vexu merged commit 8ed792b into ziglang:master Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants