-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Unexpand use buffered reads + tests #10831
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
Conversation
|
do we need that many tests ? thanks |
|
Every test is a different edge case around the chunking process. I thought rather have a few more than a few too little. |
|
GNU testsuite comparison: |
|
yeah my point was that there is some duplications in the tests and they might be merged with a loop with different args / expectations |
Merging this PR will improve performance by 58.43%
Performance Changes
Comparing Footnotes
|
| #[allow(clippy::cognitive_complexity)] | ||
| fn unexpand_line( | ||
| buf: &mut Vec<u8>, | ||
| #[allow(clippy::too_many_arguments)] |
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.
for a different PR but it should be refactored
way too many arguments now
sylvestre
left a comment
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.
still waiting for some of the tests content to be deduplicated
|
Would be 1 function like this be acceptable? |
|
sure |
This comment was marked as resolved.
This comment was marked as resolved.
|
GNU testsuite comparison: |
5c37954 to
85a2122
Compare
tests/by-util/test_unexpand.rs
Outdated
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.
Would you add keywords cspell does not like?
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.
Yes i did, i am confused why cspell marks them as error. I double checked and behaviour is written correctly. Weird!
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.
Maybe, cspell accepts en_US only? behavior
85a2122 to
5b30a37
Compare
|
GNU testsuite comparison: |
5b30a37 to
cdff1df
Compare
|
GNU testsuite comparison: |
New PR to fix issues after revert of #10798
Fix: #10698