sort: errors on overflowing -k argument but shouldn't #7188
Conversation
$ cargo test sort
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.12s
Running unittests src/bin/coreutils.rs (target/debug/deps/coreutils-7c4b7f12d70d4d85)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running tests/test_util_name.rs (target/debug/deps/test_util_name-76974a15f1516098)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 78 filtered out; finished in 0.00s
Running tests/tests.rs (target/debug/deps/tests-72f7e07ac2e9e19f)
running 131 tests
test test_comm::test_sorted ... ok
...
test test_sort::test_tmp_files_deleted_on_sigint ... ok
test result: ok. 132 passed; 0 failed; 0 ignored; 0 measured; 3162 filtered out; finished in 0.90s
|
|
GNU testsuite comparison: |
| let field = match field.parse::<usize>() { | ||
| Ok(field) => field, | ||
| Err(e) => { | ||
| match e.kind() { | ||
| IntErrorKind::PosOverflow => usize::MAX, | ||
| _ => return Err(format!("failed to parse field index {}", field.quote())), | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
more rust idiomatic:
| let field = match field.parse::<usize>() { | |
| Ok(field) => field, | |
| Err(e) => { | |
| match e.kind() { | |
| IntErrorKind::PosOverflow => usize::MAX, | |
| _ => return Err(format!("failed to parse field index {}", field.quote())), | |
| } | |
| }, | |
| }; | |
| let field = field.parse::<usize>() | |
| .unwrap_or_else(|e| match e.kind() { | |
| IntErrorKind::PosOverflow => usize::MAX, | |
| _ => panic!("failed to parse field index {}", field.quote()), | |
| }); |
There was a problem hiding this comment.
I like this better but is there a way to get away from panic! here? Assuming it would be better to unwind the error over panicing?
The contrib file also mentions we shouldn't panic.
https://github.com/uutils/coreutils/blob/main/CONTRIBUTING.md#dont-panic
There was a problem hiding this comment.
May I suggest this:
let field = match field.parse::<usize>() {
Ok(f) => f,
Err(e) if *e.kind() == IntErrorKind::PosOverflow => usize::MAX,
Err(_) => return Err(format!("failed to parse field index {}", field.quote()))
};There was a problem hiding this comment.
Oh yeah, my bad. Sorry, no crash indeed
There was a problem hiding this comment.
let field = match field.parse::<usize>() {
Ok(f) => f,
Err(e) if *e.kind() == IntErrorKind::PosOverflow => usize::MAX,
Err(e) => {
return Err(format!(
"failed to parse field index {} {}",
field.quote(),
e
))
}
};I've refactored to this. I believe this PR should be good to go now.
done |
|
GNU testsuite comparison: |
|
Before merging, please squash your commits in a single one, whose message follows this syntax In your example that could be As the commits gets merged as is in main, we better avoid having commits that don't bring anything and that just get reversed in the next one. Thank you for your contribution ! 🫶 EDIT : 2 commits actually, one being for the test with a commit message like |
rebased down to two commits, never knew git rebase interactive could do this! Should be good to go now. |
|
Beware of the new line in test_sort.rs that is added in the non-test commit. |
newline, format, and more rust idiomatic code. refactor to remove panic!
fixed, sorry about that. |
Fixes issue #7185 "sort: errors on overflowing -k argument but shouldn't"
I'm making the assumption based on the issue that we set to
usize::MAX.As this is my first ever commit to a project and new to rust, please let me know what else would have to be done if needed to get this approved.