after_parse API is underdefined and inconsistent #17
Comments
csnover
added a commit
to csnover/binrw
that referenced
this issue
Jul 2, 2023
This trait method existed only to support deferred reads of `FilePtr` values but the requirement to have cloneable arguments everywhere just to support this one use case is not justified by the number of things that it breaks. Alternative designs that do not break move-semantics are preferable, like reading offsets into a `Vec<T>` and then using `args_iter` to resolve them all at once instead. Closes jam1garner#17, jam1garner#119. Fixes jam1garner#185, jam1garner#197.
csnover
added a commit
to csnover/binrw
that referenced
this issue
Jul 2, 2023
This trait method existed only to support deferred reads of `FilePtr` values but the requirement to have cloneable arguments everywhere just to support this one use case is not justified by the number of things that it breaks. Alternative designs that do not break move-semantics are preferable, like reading offsets into a `Vec<T>` and then using `args_iter` to resolve them all at once instead. Closes jam1garner#17, jam1garner#119. Fixes jam1garner#185, jam1garner#197.
csnover
added a commit
to csnover/binrw
that referenced
this issue
Jul 2, 2023
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.
csnover
added a commit
to csnover/binrw
that referenced
this issue
Sep 18, 2023
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.
csnover
added a commit
to csnover/binrw
that referenced
this issue
Sep 19, 2023
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The documentation makes it clear that
read,read_args, orread_optionsare the correct calls to construct objects, but they don’t always construct fully-formed objects—only types generated by derive are guaranteed to callafter_parseon their children (vsVec<T>,[T;N], tuples, or other types that manually implementBinRead). Whether something was derived or not is an implementation detail which shouldn’t have extra side-effects like this. Most of the time it does not matter because most types do not useafter_parse.The text was updated successfully, but these errors were encountered: