-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
paste: permit the delimiter list to be empty #6714
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
paste: permit the delimiter list to be empty #6714
Conversation
Also: refactored the delimiter processing logic
|
GNU testsuite comparison: |
dcd0392 to
9c7381f
Compare
976b924 to
d463af0
Compare
|
GNU testsuite comparison: |
d463af0 to
e154510
Compare
|
GNU testsuite comparison: |
f2928fc to
a302d62
Compare
|
GNU testsuite comparison: |
src/uu/paste/src/paste.rs
Outdated
| // Is `map_err_context` correct here? | ||
| let file = File::open(path).map_err_context(String::new)?; |
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.
map_err_context itself looks correct to me, but not the String::new passed to it. I think it should be || name instead.
src/uu/paste/src/paste.rs
Outdated
| OneDelimiter { | ||
| delimiter: &'a [u8], | ||
| }, | ||
| MultipleDelimiters { | ||
| current_delimiter: &'a [u8], | ||
| delimiters: &'a [Box<[u8]>], | ||
| delimiters_iter: Cycle<Iter<'a, Box<[u8]>>>, | ||
| }, |
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.
Is it necessary to distinguish between a single delimiter and multiple delimiters? At least conceptually you could say there is no single delimiter, only an infinite list of delimiters.
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.
It just allows that case to be handled in a slightly cleaner/more efficient way, because we don't have to bother uselessly advancing a cycling iterator over a single element slice, and constantly recomputing the length of that single element. If you have a strong objection, I'm fine with removing it.
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.
No, I don't have a strong objection, it's fine.
11af996 to
f096a21
Compare
|
GNU testsuite comparison: |
8bb8a26 to
5a54253
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
| #[test] | ||
| fn test_non_utf8_input() { | ||
| // 0xC0 is not valid UTF-8 | ||
| const INPUT: &[u8] = b"Non-UTF-8 test: \xC0\x00\xC0.\n"; | ||
|
|
||
| new_ucmd!() | ||
| .pipe_in(INPUT) | ||
| .succeeds() | ||
| .stdout_only_bytes(INPUT); | ||
| } |
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.
Great improvement compared to the previous version with all its consts :)
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.
I had to discover "byte strings" (https://doc.rust-lang.org/stable/reference/tokens.html#characters-and-strings)!
|
Thanks! |
Also: refactored the delimiter processing logic