Skip to content

Commit

Permalink
Remove BinRead::after_parse
Browse files Browse the repository at this point in the history 
This trait method exists pretty much exclusively to support
deferred reads of `FilePtr` values but causes ongoing trouble
elsewhere and the benefits do not seem to outweight the
problems:

1. It requires `T::Args` to be cloneable in many cases where it
   should not be the case, which causes confusion, has a strong
   chance of causing accidental slowness, and makes it
   unnecessarily hard to move from imports;
2. An analysis I ran of binrw users on GitHub showed that pretty
   much all cases of `FilePtr` were using `FilePtr::parse` or
   `deref_now`, so any potential performance benefit does not
   seem to be realised by real-world projects;
3. Because there is no hard requirement to call `after_parse`
   and it mostly will not break anything, it is too easy for
   users to write custom implementations that do not do this
   and so are subtly broken. From the same GH analysis, there
   was only one case where I found someone who wrote a custom
   implementation that correctly called `after_parse`;
4. Since `after_parse` does not build a stack of objects to
   revisit later, its ability to avoid non-linear reads of data
   is limited to at most one struct or enum at a time anyway.

Given these things (and probably others that I forget), IMO
the existence of this feature is not justified. Instead, I
think that a design that reads offsets into a `Vec<{integer}>`
and then iterates over them later to convert into `Vec<T>` is
preferable; a subsequent patch includes some helper functions
to do this, but also right now it can be done (with some verbosity)
using the `args_iter` helper.

Closes jam1garner#17, jam1garner#119.
Fixes jam1garner#185, jam1garner#197.
  • Loading branch information
@csnover
csnover committed Aug 9, 2023
1 parent a4fa7fa commit 773971c
Show file tree
Hide file tree
Showing 21 changed files with 134 additions and 607 deletions.
70 changes: 5 additions & 65 deletions binrw/doc/attribute.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ Glossary of directives in binrw attributes (`#[br]`, `#[bw]`, `#[brw]`).
| r | [`count`](#count) | field | Sets the length of a vector.
| r | [`dbg`](#debug) | field | Prints the value and offset of a field to `stderr`.
| r | [`default`](#ignore) | field | An alias for `ignore`.
| r | [`deref_now`](#postprocessing) | field | An alias for `postprocess_now`.
| r | [`err_context`](#backtrace) | field | Adds additional context to errors.
| rw | [`if`](#conditional-values) | field | <span class="brw">Reads or writes</span><span class="br">Reads</span><span class="bw">Writes</span> data only if a condition is true.
| rw | [`ignore`](#ignore) | field | <span class="brw">For `BinRead`, uses the [`default`](core::default::Default) value for a field instead of reading data. For `BinWrite`, skips writing the field.</span><span class="br">Uses the [`default`](core::default::Default) value for a field instead of reading data.</span><span class="bw">Skips writing the field.</span>
Expand All @@ -100,12 +99,10 @@ Glossary of directives in binrw attributes (`#[br]`, `#[bw]`, `#[brw]`).
| rw | [`map`](#map) | all except unit variant | Maps an object or value to a new value.
| rw | [`map_stream`](#stream-access-and-manipulation) | all except unit variant | Maps the <span class="br">read</span><span class="bw">write</span> stream to a new stream.
| r | [`offset`](#offset) | field | Modifies the offset used by a [`FilePtr`](crate::FilePtr) while parsing.
| r | [`offset_after`](#offset) | field | Modifies the offset used by a [`FilePtr`](crate::FilePtr) after parsing.
| rw | [`pad_after`](#padding-and-alignment) | field | Skips N bytes after <span class="br">reading</span><span class="bw">writing</span> a field.
| rw | [`pad_before`](#padding-and-alignment) | field | Skips N bytes before <span class="br">reading</span><span class="bw">writing</span> a field.
| rw | [`pad_size_to`](#padding-and-alignment) | field | Ensures the <span class="br">reader</span><span class="bw">writer</span> is always advanced at least N bytes.
| r | [`parse_with`](#custom-parserswriters) | field | Specifies a custom function for reading a field.
| r | [`postprocess_now`](#postprocessing) | field | Calls [`after_parse`](crate::BinRead::after_parse) immediately after reading data instead of after all fields have been read.
| r | [`pre_assert`](#pre-assert) | struct, non-unit enum, unit variant | Like `assert`, but checks the condition before parsing.
| rw | [`repr`](#repr) | unit-like enum | Specifies the underlying type for a unit-like (C-style) enum.
| rw | [`restore_position`](#restore-position) | field | Restores the <span class="br">reader’s</span><span class="bw">writer’s</span> position after <span class="br">reading</span><span class="bw">writing</span> a field.
Expand Down Expand Up @@ -1828,26 +1825,21 @@ reset to where it was before parsing started.

# Offset

The `offset` and `offset_after` directives are shorthands for passing
`offset` and `offset_after` arguments to a parser that operates like
[`FilePtr`](crate::FilePtr):
The `offset` directive is shorthand for passing `offset` to a parser that
operates like [`FilePtr`](crate::FilePtr):

```text
#[br(offset = $offset:expr)] or #[br(offset($offset:expr))]
#[br(offset_after = $offset:expr)] or #[br(offset_after($offset:expr))]
```

When manually implementing
[`BinRead::read_options`](crate::BinRead::read_options) or a
[custom parser function](#custom-parserswriters), the offset is accessible
from a named argument named `offset`.

For `offset`, any earlier field or [import](#arguments) can be referenced by
the expression in the directive.

For `offset_after`, *all* fields and imports can be referenced by the
expression in the directive, but [`deref_now`](#postprocessing) cannot be
used.
Any <span class="brw">(earlier only, when reading)</span><span class="br">earlier</span>
field or [import](#arguments) can be
referenced by the expression in the directive.

## Examples

Expand Down Expand Up @@ -2056,58 +2048,6 @@ started.

<div class="br">

# Postprocessing

The `deref_now` directive, and its alias `postprocess_now`, cause a
field’s [`after_parse`](crate::BinRead::after_parse) function to be called
immediately after the field is parsed, instead of deferring the call until
the entire parent object has been parsed:

```text
#[br(deref_now)] or #[br(postprocess_now)]
```

The [`BinRead::after_parse`](crate::BinRead::after_parse) function is
normally used to perform additional work after the whole parent object has
been parsed. For example, the [`FilePtr`](crate::FilePtr) type reads an
object offset during parsing with
[`read_options`](crate::BinRead::read_options), then actually seeks to and
parses the pointed-to object in `after_parse`. This improves read
performance by reading the whole parent object sequentially before seeking
to read the pointed-to object.

However, if another field in the parent object needs to access data from the
pointed-to object, `after_parse` needs to be called earlier. Adding
`deref_now` (or its alias, `postprocess_now`) to the earlier field causes
this to happen.

## Examples

```
# use binrw::{prelude::*, FilePtr32, NullString, io::Cursor};
#[derive(BinRead, Debug)]
#[br(big, magic = b"TEST")]
struct TestFile {
#[br(deref_now)]
ptr: FilePtr32<NullString>,
value: i32,
// Notice how `ptr` can be used as it has already been postprocessed
#[br(calc = ptr.len())]
ptr_len: usize,
}
# let test_contents = b"\x54\x45\x53\x54\x00\x00\x00\x10\xFF\xFF\xFF\xFF\x00\x00\x00\x00\x54\x65\x73\x74\x20\x73\x74\x72\x69\x6E\x67\x00\x00\x00\x00\x69";
# let test = Cursor::new(test_contents).read_be::<TestFile>().unwrap();
# assert_eq!(test.ptr_len, 11);
# assert_eq!(test.value, -1);
# assert_eq!(test.ptr.to_string(), "Test string");
```
</div>

<div class="br">

# Pre-assert

`pre_assert` works like [`assert`](#assert), but checks the condition before
Expand Down
60 changes: 0 additions & 60 deletions binrw/src/binread/impls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,22 +144,6 @@ where
) -> BinResult<Self> {
crate::helpers::count_with(args.count, B::read_options)(reader, endian, args.inner)
}

fn after_parse<R>(
&mut self,
reader: &mut R,
endian: Endian,
args: Self::Args<'_>,
) -> BinResult<()>
where
R: Read + Seek,
{
for val in self.iter_mut() {
val.after_parse(reader, endian, args.inner.clone())?;
}

Ok(())
}
}

impl<B, const N: usize> BinRead for [B; N]
Expand All @@ -176,22 +160,6 @@ where
) -> BinResult<Self> {
array_init::try_array_init(|_| BinRead::read_options(reader, endian, args.clone()))
}

fn after_parse<R>(
&mut self,
reader: &mut R,
endian: Endian,
args: Self::Args<'_>,
) -> BinResult<()>
where
R: Read + Seek,
{
for val in self.iter_mut() {
val.after_parse(reader, endian, args.clone())?;
}

Ok(())
}
}

macro_rules! binread_tuple_impl {
Expand All @@ -208,19 +176,6 @@ macro_rules! binread_tuple_impl {
),*
))
}

fn after_parse<R: Read + Seek>(&mut self, reader: &mut R, endian: Endian, args: Self::Args<'_>) -> BinResult<()> {
let ($type1, $(
$types
),*) = self;

$type1.after_parse(reader, endian, args.clone())?;
$(
$types.after_parse(reader, endian, args.clone())?;
)*

Ok(())
}
}

binread_tuple_impl!($($types),*);
Expand Down Expand Up @@ -264,21 +219,6 @@ impl<T: BinRead> BinRead for Option<T> {
) -> BinResult<Self> {
Ok(Some(T::read_options(reader, endian, args)?))
}

fn after_parse<R>(
&mut self,
reader: &mut R,
endian: Endian,
args: Self::Args<'_>,
) -> BinResult<()>
where
R: Read + Seek,
{
match self {
Some(val) => val.after_parse(reader, endian, args),
None => Ok(()),
}
}
}

impl<T> BinRead for core::marker::PhantomData<T> {
Expand Down
41 changes: 9 additions & 32 deletions binrw/src/binread/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,22 +160,6 @@ pub trait BinRead: Sized {
endian: Endian,
args: Self::Args<'_>,
) -> BinResult<Self>;

/// Runs any post-processing steps required to finalize construction of the
/// object.
///
/// # Errors
///
/// If post-processing fails, an [`Error`](crate::Error) variant will be
/// returned.
fn after_parse<R: Read + Seek>(
&mut self,
_: &mut R,
_: Endian,
_: Self::Args<'_>,
) -> BinResult<()> {
Ok(())
}
}

/// Extension methods for reading [`BinRead`] objects directly from a reader.
Expand All @@ -202,7 +186,7 @@ pub trait BinReaderExt: Read + Seek + Sized {
fn read_type<'a, T>(&mut self, endian: Endian) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Required + Clone,
T::Args<'a>: Required,
{
self.read_type_args(endian, T::Args::args())
}
Expand All @@ -216,7 +200,7 @@ pub trait BinReaderExt: Read + Seek + Sized {
fn read_be<'a, T>(&mut self) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Required + Clone,
T::Args<'a>: Required,
{
self.read_type(Endian::Big)
}
Expand All @@ -230,7 +214,7 @@ pub trait BinReaderExt: Read + Seek + Sized {
fn read_le<'a, T>(&mut self) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Required + Clone,
T::Args<'a>: Required,
{
self.read_type(Endian::Little)
}
Expand All @@ -244,7 +228,7 @@ pub trait BinReaderExt: Read + Seek + Sized {
fn read_ne<'a, T>(&mut self) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Required + Clone,
T::Args<'a>: Required,
{
self.read_type(Endian::NATIVE)
}
Expand All @@ -254,15 +238,11 @@ pub trait BinReaderExt: Read + Seek + Sized {
/// # Errors
///
/// If reading fails, an [`Error`](crate::Error) variant will be returned.
fn read_type_args<'a, T>(&mut self, endian: Endian, args: T::Args<'a>) -> BinResult<T>
fn read_type_args<T>(&mut self, endian: Endian, args: T::Args<'_>) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Clone,
{
let mut res = T::read_options(self, endian, args.clone())?;
res.after_parse(self, endian, args)?;

Ok(res)
T::read_options(self, endian, args)
}

/// Read `T` from the reader, assuming big-endian byte order, using the
Expand All @@ -272,10 +252,9 @@ pub trait BinReaderExt: Read + Seek + Sized {
///
/// If reading fails, an [`Error`](crate::Error) variant will be returned.
#[inline]
fn read_be_args<'a, T>(&mut self, args: T::Args<'a>) -> BinResult<T>
fn read_be_args<T>(&mut self, args: T::Args<'_>) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Clone,
{
self.read_type_args(Endian::Big, args)
}
Expand All @@ -287,10 +266,9 @@ pub trait BinReaderExt: Read + Seek + Sized {
///
/// If reading fails, an [`Error`](crate::Error) variant will be returned.
#[inline]
fn read_le_args<'a, T>(&mut self, args: T::Args<'a>) -> BinResult<T>
fn read_le_args<T>(&mut self, args: T::Args<'_>) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Clone,
{
self.read_type_args(Endian::Little, args)
}
Expand All @@ -302,10 +280,9 @@ pub trait BinReaderExt: Read + Seek + Sized {
///
/// If reading fails, an [`Error`](crate::Error) variant will be returned.
#[inline]
fn read_ne_args<'a, T>(&mut self, args: T::Args<'a>) -> BinResult<T>
fn read_ne_args<T>(&mut self, args: T::Args<'_>) -> BinResult<T>
where
T: BinRead,
T::Args<'a>: Clone,
{
self.read_type_args(Endian::NATIVE, args)
}
Expand Down
Loading

0 comments on commit 773971c

Please sign in to comment.