tr: Add ambiguous octal escape warning #6886
Conversation
OshinoShinobu-Chan
force-pushed
the
main
branch
from
e801364 to
30a4724
Compare
|
GNU testsuite comparison: |
OshinoShinobu-Chan
force-pushed
the
main
branch
from
30a4724 to
6954ad9
Compare
src/uu/tr/src/operation.rs
Outdated
| match u8::from_str_radix(str_to_parse, 8) { | ||
| Ok(ue) => Some(ue), | ||
| Err(_pa) => None, | ||
| } |
There was a problem hiding this comment.
You can simplify this and use ok() to turn the Result into an Option:
| match u8::from_str_radix(str_to_parse, 8) { | |
| Ok(ue) => Some(ue), | |
| Err(_pa) => None, | |
| } | |
| u8::from_str_radix(str_to_parse, 8).ok() |
There was a problem hiding this comment.
i wonder if there is a clippy warning for this?!
There was a problem hiding this comment.
@sylvestre I'm not aware of such a lint.
There was a problem hiding this comment.
Yes, you are right. I just copied it and removed the previous TODO comment, so I didn't notice that this match is redundant.
There was a problem hiding this comment.
@samueltardieu you might be interested by this :)
There was a problem hiding this comment.
Indeed, this is lacking. I'll add this to my TODO list.
There was a problem hiding this comment.
src/uu/tr/src/operation.rs
Outdated
| let str_to_parse = std::str::from_utf8(out).unwrap(); | ||
| match u8::from_str_radix(str_to_parse, 8) { | ||
| Ok(ue) => Some(ue), | ||
| Err(_pa) => { |
There was a problem hiding this comment.
I think it is cleaner to simply use Err(_):
| Err(_pa) => { | |
| Err(_) => { |
There was a problem hiding this comment.
The similar reason to the one above, and I agree with this change too. Thank you for your review and suggestions.
src/uu/tr/src/operation.rs
Outdated
| match u8::from_str_radix(str_to_parse, 8) { | ||
| Ok(ue) => Some(ue), | ||
| Err(_pa) => { | ||
| // TODO | ||
| // A warning needs to be printed here | ||
| // See https://github.com/uutils/coreutils/issues/6821 | ||
| let origin_octal: &str = std::str::from_utf8(input).unwrap(); | ||
| let actual_octal_tail: &str = std::str::from_utf8(&input[0..2]).unwrap(); | ||
| let outstand_char: char = char::from_u32(input[2] as u32).unwrap(); | ||
| show_warning!( | ||
| "the ambiguous octal escape \\{} is being\n interpreted as the 2-byte sequence \\0{}, {}", | ||
| origin_octal, | ||
| actual_octal_tail, | ||
| outstand_char | ||
| ); | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
Here it might be an option to use something like:
let result = u8::from_str_radix(str_to_parse, 8).ok();
if result.is_none() {
let origin_octal: &str = std::str::from_utf8(input).unwrap();
let actual_octal_tail: &str = std::str::from_utf8(&input[0..2]).unwrap();
let outstand_char: char = char::from_u32(input[2] as u32).unwrap();
show_warning!(
"the ambiguous octal escape \\{} is being\n interpreted as the 2-byte sequence \\0{}, {}",
origin_octal,
actual_octal_tail,
outstand_char
);
}
resultThis way you could remove one indentation level.
There was a problem hiding this comment.
I don't think this is necessary. But it does make the code looks nicer. I think I'll accept this suggestion.
|
@OshinoShinobu-Chan Thanks for your PR :) |
changelog: [`manual_ok_err`]: new lint
Detect manual implementations of `.ok()` or `.err()`, as in
```rust
let a = match func() {
Ok(v) => Some(v),
Err(_) => None,
};
let b = if let Err(v) = func() {
Some(v)
} else {
None
};
```
which can be replaced by
```rust
let a = func().ok();
let b = func().err();
```
This pattern was detected in the wild in the Rust reimplementation of
coreutils:
uutils/coreutils#6886 (review)
4 participants
This PR is to fix issue #6821.
I add
parse_octal_up_to_three_digits_with_warningwhich is similar toparse_octal_up_to_three_digitsbut can print warning. Becuaseparse_octal_up_to_three_digitsis alse used in other cases, and those cases should not output the warning. With two versions, we can avoid printing the same warning more than once.Some of the other functions are also added "with_warning" version.