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.

Sign up for GitHub

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

Impl from_reader #352

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Impl from_reader #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
name: Coverage

on: [pull_request, push]

jobs:
coverage:
runs-on: ubuntu-latest
env:
CARGO_TERM_COLOR: always
steps:
- uses: actions/checkout@v3
- name: Install Rust
run: rustup update stable
- name: Install cargo-llvm-cov
uses: taiki-e/install-action@cargo-llvm-cov
- name: Generate code coverage
run: cargo llvm-cov --workspace --codecov --output-path codecov.json
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v3
with:
files: codecov.json
fail_ci_if_error: true
69 changes: 27 additions & 42 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,30 +33,31 @@ jobs:
command: test
args: --all

test_miri:
name: Miri Test
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions-rs/toolchain@v1
with:
toolchain: nightly
override: true
components: miri
- run: cargo miri test

test_miri_big_endian:
name: Miri Test Big Endian
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions-rs/toolchain@v1
with:
toolchain: nightly
override: true
components: miri
target: mips64-unknown-linux-gnuabi64
- run: cargo miri test --target mips64-unknown-linux-gnuabi64
# TODO: Enable Miri
# test_miri:
# name: Miri Test
# runs-on: ubuntu-latest
# steps:
# - uses: actions/checkout@v3
# - uses: actions-rs/toolchain@v1
# with:
# toolchain: nightly
# override: true
# components: miri
# - run: cargo miri test
#
# test_miri_big_endian:
# name: Miri Test Big Endian
# runs-on: ubuntu-latest
# steps:
# - uses: actions/checkout@v3
# - uses: actions-rs/toolchain@v1
# with:
# toolchain: nightly
# override: true
# components: miri
# target: armebv7r-none-eabi
# - run: cargo miri test --target armebv7r-none-eabi

examples:
name: Examples
Expand Down Expand Up @@ -111,7 +112,8 @@ jobs:
with:
toolchain: nightly
override: true
- run: cd ensure_no_std && cargo run --release
target: thumbv7em-none-eabihf
- run: cd ensure_no_std && cargo build --release --target thumbv7em-none-eabihf

ensure_wasm:
name: Ensure wasm
Expand All @@ -126,20 +128,3 @@ jobs:
with:
version: 'latest'
- run: cd ensure_wasm && wasm-pack build --target web && wasm-pack test --node

coverage:
name: Coverage
runs-on: ubuntu-latest
container:
image: xd009642/tarpaulin:develop
options: --security-opt seccomp=unconfined
steps:
- name: Checkout repository
uses: actions/checkout@v3

- name: Generate code coverage
run: |
cargo tarpaulin --verbose --all-features --workspace --timeout 120 --out Xml

- name: Upload to codecov.io
uses: codecov/codecov-action@v1
129 changes: 129 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,135 @@

## [Unreleased]

## Changes
[#352](https://github.com/sharksforarms/deku/pull/352) added a new function `from_reader` that uses `io::Read`.
`io::Read` is also now used internally, bringing massive performance and usability improvements.

### New `from_reader`
```rust
use std::io::{Seek, SeekFrom, Read};
use std::fs::File;
use deku::prelude::*;

#[derive(Debug, DekuRead, DekuWrite, PartialEq, Eq, Clone, Hash)]
#[deku(endian = "big")]
struct EcHdr {
magic: [u8; 4],
version: u8,
padding1: [u8; 3],
}

let mut file = File::options().read(true).open("file").unwrap();
let ec = EcHdr::from_reader((&mut file, 0)).unwrap();
```

- The more internal (with context) `read(..)` was replaced with `from_reader_with_ctx(..)`.
With the switch to internal streaming, the variables `deku::input`, `deku::input_bits`, and `deku::rest` are now not possible and were removed.
`deku::reader` is a replacement for some of the functionality.
See [examples/deku_input.rs](examples/deku_input.rs) for a new example of caching all reads.

old:
```rust
#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct DekuTest {
field_a: u8,

#[deku(
reader = "bit_flipper_read(*field_a, deku::rest, BitSize(8))",
)]
field_b: u8,
}

fn custom_read(
field_a: u8,
rest: &BitSlice<u8, Msb0>,
bit_size: BitSize,
) -> Result<(&BitSlice<u8, Msb0>, u8), DekuError> {

// read field_b, calling original func
let (rest, value) = u8::read(rest, bit_size)?;

Ok((rest, value))
}
```

new:
```rust
#[derive(Debug, PartialEq, DekuRead, DekuWrite)]
struct DekuTest {
field_a: u8,

#[deku(
reader = "bit_flipper_read(*field_a, deku::reader, BitSize(8))",
)]
field_b: u8,
}

fn custom_read<R: std::io::Read>(
field_a: u8,
reader: &mut Reader<R>,
bit_size: BitSize,
) -> Result<u8, DekuError> {

// read field_b, calling original func
let value = u8::from_reader_with_ctx(reader, bit_size)?;

Ok(value)
}
```

- With the addition of using `Read`, containing a byte slice with a reference is not supported:
Copy link
Owner

Choose a reason for hiding this comment

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

Makes sense, although some users are probably using references like this as a zero-copy optimization, these would now require an allocation. This is an API trade-off which we can explain here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For sure, i'll add a comment.

Copy link
Collaborator Author

@wcampbell0x2a wcampbell0x2a Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Interestingly, the following works:

#[derive(Debug)]
struct Nice<'a> {
    buf: &'a [u8]
}

impl<'a> Nice<'a> {
    pub fn from_reader(mut reader: impl std::io::Read, storage: &'a mut [u8]) -> Nice<'a> {
        reader.read_exact(storage);
       
        Nice {
            buf: storage,
        }
    }
}

fn main() {
    let mut buf = std::io::Cursor::new([0x00, 0x01, 0x02]);
    let mut storage = [0; 3];
    let nice = Nice::from_reader(&mut buf, &mut storage);
    dbg!(nice);
}

I haven't tried it at the scale of deku..


old
```rust
#[derive(PartialEq, Debug, DekuRead, DekuWrite)]
struct TestStruct<'a> {
bytes: u8,

#[deku(bytes_read = "bytes")]
data: &'a [u8],
}
```

new
```rust
#[derive(PartialEq, Debug, DekuRead, DekuWrite)]
struct TestStruct {
bytes: u8,

#[deku(bytes_read = "bytes")]
data: Vec<u8>,
}
```

- `id_pat` is now required to be the same type as stored id.
This also disallows using tuples for storing the id:
Copy link
Owner

Choose a reason for hiding this comment

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

Bit confused as to why we can't use tuples? do you mind summarizing for me?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comments:

old:

   impl ::deku::DekuRead<'_, ()> for DekuTest {
        fn read(
            __deku_input_bits: &'_ ::deku::bitvec::BitSlice<u8, ::deku::bitvec::Msb0>,
            _: (),
        ) -> core::result::Result<
            (&'_ ::deku::bitvec::BitSlice<u8, ::deku::bitvec::Msb0>, Self),
            ::deku::DekuError,
        > {
            use core::convert::TryFrom;
            let mut __deku_rest = __deku_input_bits;
            // in this version, we read from __deku_rest, but don't use __deku_new_rest,
            // so we don't move the "reader"
            let (__deku_new_rest, __deku_variant_id) = <u8>::read(__deku_rest, ())?;
            let __deku_value = match &__deku_variant_id {
                _ => {
                    let __deku_field_0 = {
                        let (__deku_new_rest, __deku_value) = <(
                            u8,
                            u8,
                        ) as ::deku::DekuRead<'_, _>>::read(__deku_rest, ())?;
                        let __deku_value: (u8, u8) = core::result::Result::<
                            _,
                            ::deku::DekuError,
                        >::Ok(__deku_value)?;
                        __deku_rest = __deku_new_rest;
                        __deku_value
                    };
                    let field_0 = &__deku_field_0;
                    Self::VariantC(__deku_field_0)
                }
            };
            Ok((__deku_rest, __deku_value))
        }
    }

new:

   impl ::deku::DekuReader<'_, ()> for DekuTest {
        fn from_reader_with_ctx<R: ::deku::no_std_io::Read>(
            __deku_reader: &mut ::deku::reader::Reader<R>,
            _: (),
        ) -> core::result::Result<Self, ::deku::DekuError> {
            use core::convert::TryFrom;
            let __deku_variant_id = <u8>::from_reader_with_ctx(__deku_reader, ())?;
            let __deku_value = match &__deku_variant_id {
                _ => {
                    let __deku_field_0 = {
                        // we just use the __deku_variant_id
                        let __deku_value = __deku_variant_id;
                        let __deku_value: (u8, u8) = core::result::Result::<
                            _,
                            ::deku::DekuError,
                        >::Ok(__deku_value)?;
                        __deku_value
                    };
                    let field_0 = &__deku_field_0;
                    Self::VariantC(__deku_field_0)
                }
            };
            Ok(__deku_value)
        }
    }

I can't clone the reader, and caching the exact `

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This becomes an easy problem in the case of adding the Seek trait on reader, hard if not. As we need to create the exact state of reader before reading __deku_variant_id.


old:
```rust
#[derive(PartialEq, Debug, DekuRead, DekuWrite)]
#[deku(type = "u8")]
enum DekuTest {
#[deku(id_pat = "_")]
VariantC((u8, u8)),
}
```

new:
```rust
#[derive(PartialEq, Debug, DekuRead, DekuWrite)]
#[deku(type = "u8")]
enum DekuTest {
#[deku(id_pat = "_")]
VariantC {
id: u8,
other: u8,
},
}
```

- The feature `const_generics` was removed and is enabled by default.

## [0.16.0] - 2023-02-28

### Changes
Expand Down
7 changes: 4 additions & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ keywords = ["deku", "bits", "serialization", "deserialization", "struct"]
categories = ["encoding", "parsing", "no-std"]
description = "bit level serialization/deserialization proc-macro for structs"
readme = "README.md"
rust-version = "1.65.0"

[lib]
bench = false
Expand All @@ -19,16 +20,16 @@ members = [
]

[features]
default = ["std", "const_generics"]
std = ["deku_derive/std", "bitvec/std", "alloc"]
default = ["std"]
std = ["deku_derive/std", "bitvec/std", "alloc", "no_std_io/std"]
alloc = ["bitvec/alloc"]
logging = ["deku_derive/logging", "log"]
const_generics = []

[dependencies]
deku_derive = { version = "^0.16.0", path = "deku-derive", default-features = false}
bitvec = { version = "1.0.1", default-features = false }
log = { version = "0.4.17", optional = true }
no_std_io = { version = "0.5.0", default-features = false, features = ["alloc"] }

[dev-dependencies]
rstest = "0.16.0"
Expand Down
Loading