tail: Refactor paths::Input::from and Settings::inputs
#4756
Conversation
…c instead of VecDeque
Joining7943
changed the title
: Refactor paths::Input::from and Settings::inputs`tail: Refactor paths::Input::from and Settings::inputs
src/uu/tail/src/args.rs
Outdated
| .map(|v| v.map(|string| Input::from(&string)).collect()) | ||
| .unwrap_or_default(); | ||
| settings.inputs = matches | ||
| .get_raw(options::ARG_FILES) |
There was a problem hiding this comment.
Can we do this without get_raw? Maybe with an OsStr or Path value parser?
There was a problem hiding this comment.
What's the disadvantage of get_raw?
There was a problem hiding this comment.
Not sure there's a real disadvantage, but it feels like a workaround. Something that should only be necessary if you have parsed values and want to access the raw value as well.
There was a problem hiding this comment.
I'm pretty sure it's meant as the direct way to access the parsed values as OsStr ;) imho value parsing to get an OsStr from clap is just a waste of resources.
There was a problem hiding this comment.
I'd be surprised if it actually did any parsing. It's just gonna expect an OsStr, which always succeeds and then can give us an OsStr as a value.
There was a problem hiding this comment.
Can we do it the easy and direct way, please? From the docs of clap get_raw
Iterate over the original argument values.
With get_raw, there's nothing else between us and the OsStr, neither during compile time nor runtime and the code is clearly expressing that intent.
There was a problem hiding this comment.
That intent is exactly what an OsStr value parser expresses and it does it i n the clap builder, instead of at this location. It is in my opinion the most easy and direct way, because it uses claps normal parsing.
Besides: clap-rs/clap#3809
There was a problem hiding this comment.
That clap issue is open waiting for decision since last year, so I think we can safely use it. Can we settle this please? If you insist I'll change it somehow, but I really don't think it's worth it.
There was a problem hiding this comment.
Fine. Let's hope I can make a better API than clap's if I ever find time to work on my own parser again :)
|
I wasn't very happy with the comparison with a new OsStr in UPDATE: It's changed now |
Joining7943
force-pushed
the
tail-refactor-and-fix-input
branch
from
cb0d725
to
ae60045
Compare
|
@tertsdiepraam Just a quick question by the way. In the |
|
I usually prefer |
Ok, then let's change them to |
src/uu/tail/src/paths.rs
Outdated
| #[cfg(unix)] | ||
| impl From<&OsStr> for InputKind { | ||
| fn from(value: &OsStr) -> Self { | ||
| const DASH: [u8; 1] = [b'-']; |
There was a problem hiding this comment.
I'm not sure, but I think this can be a b"-"?
There was a problem hiding this comment.
Yeah, but it's just a single character. It doesn't play a big role here but the result is an array [u8; 1] instead of a reference to an array &[u8; 1].
There was a problem hiding this comment.
So you don't think
const DASH: &[u8] = b"-";
if value.as_bytes() == DASH { }is clearer?
There was a problem hiding this comment.
No not really, to me both look very similar and equally clear. However, the correct type of a byte string literal is &[u8; n] or better &'static [u8; n]. I first used b"-" but then changed it to [b'-'] to spare me the reference here and I'm sure that DASH is stored on the stack. We're talking here about a single character. If you have a longer string literal and just need it as a (reference to a) byte array than you might spare yourself a lot of typing by using a byte string literal.
There was a problem hiding this comment.
Actually, I think I'd actually prefer this:
let DASH = OsStr::new("-");
if value == DASH { }Then we don't even need to think about byte strings.
There was a problem hiding this comment.
If you're worried about performance, they all generate the same assembly: https://godbolt.org/z/M66sjEqGe
There was a problem hiding this comment.
OsStr::new can't be const. Why can't we just keep that little optimization the way it is and noone needs to look at any assembly code and rely on compiler magic?
There was a problem hiding this comment.
No, my priority here is clear code. I'd still push for the change even if it was technically slower, because it's not worth optimizing. The current code harder to read without any advantage.
There was a problem hiding this comment.
Outside observer looking through some PRs: I think OsStr is the nicer option here, it's clearer what's going on. [b'-'] is hard to read. And the performance seems to be the same anyway even though if the performance were different, it would not matter.
|
@tertsdiepraam Sorry, these kind of code reviews lately are boring me. Take this pr or leave it. If you decide for the first you should notice some improvements in code quality. |
I'm sorry to hear that. I'll take a look at the PR and thanks for your work on this in any case! |
|
I've added a small commit with my last nitpicky comments :) @sylvestre or @cakebaker could you review it? |
|
LGTM, thanks |
This pr refactors mainly the
Input::frommethod and changes inputs inSettingsfrom aVecDequeto aVec.There is no need for
Settings::inputsbeing aVecDequeanymore andVecis a little bit more efficient. InInput::from, unpackingAsRef<OsString>with.as_ref()just once is also a bit more efficient. Same with the comparison ofOsStrwith anotherOsStrinstead ofPath. Parsing the input arguments to aStringand then back to anOsStris unnecessary, since we can get the inputs raw asOsStrfrom clap to pass them toInput::from.Note this pr also includes a small fix. When file headers are printed in verbose mode, the output for stdin should be equal on all platforms:
==> standard input <==. Before this change the output for non unix systems was==> - <==.