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

Add Little Endian variants for Read/WriteExt #1915

Merged
merged 5 commits into from Jul 16, 2020

Conversation

leshow
Copy link
Contributor

@leshow leshow commented Dec 6, 2019

Motivation

There are functions to read in big endian format but not little endian, as mentioned on reddit.

Solution

Add read/write methods to AsyncReadExt and AsyncWriteExt that allow reading/writing in little endian.

Looks like these methods are all generated with macros, I'll add *Le variants of all the read/write types and then implement them in AsyncReadExt and AsyncWriteExt respectively.

I'm submitting a WIP MR to make sure I'm on the right track, I'll complete the docs and AsyncWriteExt shortly.

Tests are now fixed and everything appears to work correctly. I kept the test input the same between le and be variants.

@leshow leshow changed the title WIP: Add Little Endian variants for Read/WriteExt Add Little Endian variants for Read/WriteExt Dec 7, 2019
@Kobzol
Copy link
Contributor

Kobzol commented Dec 7, 2019

Could this be parametrized with a type argument, same as in https://docs.rs/byteorder/1.1.0/byteorder/trait.ReadBytesExt.html?

@leshow
Copy link
Contributor Author

leshow commented Dec 7, 2019

The underlying crate tokio uses is the bytes crate, from which I copied the naming convention https://docs.rs/bytes/0.5.2/bytes/buf/trait.Buf.html#method.get_u16_le

This is the same naming that is used in std as well with methods like from_le

If you decide to move in a different direction, I can change this PR, but I think following the familiar naming convention is probably a good idea.

@Kobzol
Copy link
Contributor

Kobzol commented Dec 7, 2019

Ok, you're right, byteorder does it differently, but std and bytes use this convention, so it's a better idea for tokio as well I guess. Thanks for the PR btw! I need it in my use case.

@leshow
Copy link
Contributor Author

leshow commented Jan 9, 2020

Any update on this?

@zebp
Copy link

zebp commented Jul 14, 2020

Can this get a review? This seems super handy.

@leshow
Copy link
Contributor Author

leshow commented Jul 14, 2020

If you need me to rebase and update it, give me a heads up.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io labels Jul 14, 2020
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

@Darksonn
Copy link
Contributor

It is preferred that you simply make a merge commit instead of rebasing.

@leshow
Copy link
Contributor Author

leshow commented Jul 15, 2020

Fair enough. Merged with master, everything builds fine, but I ran into some very odd errors during cargo test

@hawkw
Copy link
Member

hawkw commented Jul 15, 2020

Fair enough. Merged with master, everything builds fine, but I ran into some very odd errors during cargo test

@leshow what errors are you seeing? It looks like the tests passed for your branch on CI...

@leshow
Copy link
Contributor Author

leshow commented Jul 15, 2020

This is running from the git root dir. If I run from tokio subdir, all the tests pass

❯ cargo test --all-features            
   Compiling smallvec v1.2.0
   Compiling scopeguard v1.0.0
   Compiling lock_api v0.3.3
   Compiling parking_lot_core v0.7.0
   Compiling parking_lot v0.10.0
   Compiling tokio v0.2.21 (/home/leshow/dev/rust/tokio/tokio)
   Compiling tokio-test v0.2.1 (/home/leshow/dev/rust/tokio/tokio-test)
   Compiling tokio-util v0.3.1 (/home/leshow/dev/rust/tokio/tokio-util)
   Compiling tests-build v0.1.0 (/home/leshow/dev/rust/tokio/tests-build)
   Compiling tests-integration v0.1.0 (/home/leshow/dev/rust/tokio/tests-integration)
   Compiling examples v0.0.0 (/home/leshow/dev/rust/tokio/examples)
   Compiling tokio-macros v0.2.5 (/home/leshow/dev/rust/tokio/tokio-macros)
    Finished test [unoptimized + debuginfo] target(s) in 55.82s
     Running target/debug/deps/tests_build-d99f5f3e0262922b

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/debug/deps/macros-6f38e01ab371efcf

running 1 test
    Updating crates.io index
error: failed to select a version for `proc-macro2`.
    ... required by package `tokio-macros v0.2.5 (/home/leshow/dev/rust/tokio/tokio-macros)`
    ... which is depended on by `tokio v0.2.21 (/home/leshow/dev/rust/tokio/tokio)`
    ... which is depended on by `tests-build v0.1.0 (/home/leshow/dev/rust/tokio/tests-build)`
    ... which is depended on by `tests-build-tests v0.0.0 (/home/leshow/dev/rust/tokio/target/tests/tests-build)`
versions that meet the requirements `^1.0.7` are: 1.0.18, 1.0.17, 1.0.16, 1.0.15, 1.0.14, 1.0.13, 1.0.12, 1.0.10, 1.0.9, 1.0.8, 1.0.7

all possible versions conflict with previously selected packages.

  previously selected package `proc-macro2 v1.0.6`
    ... which is depended on by `quote v1.0.2`
    ... which is depended on by `syn v1.0.11`
    ... which is depended on by `tokio-macros v0.2.5 (/home/leshow/dev/rust/tokio/tokio-macros)`
    ... which is depended on by `tokio v0.2.21 (/home/leshow/dev/rust/tokio/tokio)`
    ... which is depended on by `tests-build v0.1.0 (/home/leshow/dev/rust/tokio/tests-build)`
    ... which is depended on by `tests-build-tests v0.0.0 (/home/leshow/dev/rust/tokio/target/tests/tests-build)`

failed to select a version for `proc-macro2` which could resolve this conflict
test compile_fail ... FAILED

failures:

---- compile_fail stdout ----
thread 'compile_fail' panicked at 'tests failed', /home/leshow/.cargo/registry/src/github.com-1ecc6299db9ec823/trybuild-1.0.18/src/run.rs:38:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    compile_fail

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '-p tests-build --test macros'

This is the local failure I'm seeing

@Darksonn Darksonn merged commit 7e4edb8 into tokio-rs:master Jul 16, 2020
hawkw added a commit that referenced this pull request Jul 20, 2020
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2572, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)

- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- io: fix panic in `AsyncReadExt::read_line` (#2541)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 22, 2020
# 0.2.22 (July 2!, 2020)

### Fixes
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)
- io: fix panic in `AsyncReadExt::read_line` (#2541)

### Changes
- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

### Added
- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants