Skip to content
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

Option for reading between two offsets #75

Closed
wants to merge 5 commits into from
Closed

Option for reading between two offsets #75

wants to merge 5 commits into from

Conversation

MaxJohansen
Copy link

@MaxJohansen MaxJohansen commented Oct 11, 2019

I have tried to implement the syntax suggested in the discussion on issue #16.

$ hexyl --range 512:1024 # Means "start at 512th byte, read until byte 1024 (512 bytes)"
$ hexyl --range 512:+512 # Means "start at 512th byte, read 512 bytes"

$ hexyl --range +512:    # Means "start at 512th byte".

I did not add support for negative offsets simply because I do not see a simple way to implement it for Stdin. If required, I suppose it could be implemented with a buffering solution that reads M - N bytes into a buffer until it reaches the end of Stdin before passing it to a reader.

My solution differs from #43 by not modifying the Printer itself, merely the Reader that it receives. This means that it works just as well for Stdin as for a file. A consequence of this is that the bytes are no longer automatically "aligned" in the output, but that just might be what you want in this situation.

$ hexyl -r :+0x20 hexyl
┌────────┬─────────────────────────┬─────────────────────────┬────────┬────────┐
│00000000│ 7f 45 4c 46 02 01 01 00 ┊ 00 00 00 00 00 00 00 00 │•ELF•••0┊00000000│
│00000010│ 03 00 3e 00 01 00 00 00 ┊ 30 d1 02 00 00 00 00 00 │•0>0•000┊0ו00000│
└────────┴─────────────────────────┴─────────────────────────┴────────┴────────┘

$ hexyl -r 0x3:+0x20 hexyl
┌────────┬─────────────────────────┬─────────────────────────┬────────┬────────┐
│00000003│ 46 02 01 01 00 00 00 00 ┊ 00 00 00 00 00 03 00 3e │F•••0000┊00000•0>│
│00000013│ 00 01 00 00 00 30 d1 02 ┊ 00 00 00 00 00 40 00 00 │0•0000ו┊00000@00│
└────────┴─────────────────────────┴─────────────────────────┴────────┴────────┘

This is my first contribution of Rust code and I welcome suggestions that can help me improve! :)

@MaxJohansen MaxJohansen mentioned this pull request Oct 17, 2019
6 tasks
@sharkdp
Copy link
Owner

sharkdp commented Oct 21, 2019

Thank you very much for your contribution!

From a first glance, the changes look great, but I didn't have the time for a full review yet. Could be a few more days.

Comment on lines +137 to +139
let mut discard = vec![0u8; offset as usize];
reader
.read_exact(&mut discard)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that seems a bit hacky, but I also couldn't find a better solution after a quick search.

I'm not sure if we should leave it like this. If somebody wants to skip the first few MB or even GB of a file/input, we would actually allocate a buffer of this size on the heap which could easily fail. Even if it doesn't fail, it seems wrong to allocate a huge buffer for simply discarding everything inside it.

If that helps, I think we could actually use the BufRead trait instead of just Read. We would simply need to create a BufReader for the input file. The StdinLock already implements BufRead.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bit of research on this and Stdin doesn't appear to be seekable no matter what I do. I'll continue exploring other options for this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe read to a 1024 bytes or so big slice multiple times as necessary and then read to a smaller slice as necessary.

src/bin/hexyl.rs Outdated Show resolved Hide resolved
@seeplusplus seeplusplus mentioned this pull request Mar 12, 2020
@Tarnadas Tarnadas mentioned this pull request Apr 29, 2020
@sharkdp
Copy link
Owner

sharkdp commented May 26, 2020

I'm going to close this for now, with #88 merged. Happy to reopen it if there is any further comments/progress.

@sharkdp sharkdp closed this May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants