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
Field-level map_stream
fails to compile
#197
Comments
map_stream
does not workmap_stream
fails to compile
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.
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.
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.
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.
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.
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.
This is fixed by 13525e3. |
the following code still seems to break for me on 0.12. seems like this may be a different issue? I assume it breaks because of the closure from
|
@super-continent Sorry about the delay. What you are reporting was fixed by 97ce5be and will be working correctly in 0.13. For the moment you can switch to a git checkout to get the fix. A new release should happen soon. |
So I was playing around with
map_stream
and it seems that this simple example fails to compile:It gives:
If you look at the output of cargo-expand:
The text was updated successfully, but these errors were encountered: