Remove BinRead::after_parse
and add offset-table helpers
#210
+359
−637
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This trait method exists pretty much exclusively to support deferred reads of
FilePtr
values but causes ongoing troubleelsewhere and the benefits do not seem to outweigh the problems:
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;FilePtr
were usingFilePtr::parse
orderef_now
, so any potential performance benefit does not seem to be realised by real-world projects;after_parse
and it mostly will not break anything, it is too easy forusers 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
;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 intoVec<T>
ispreferable.
To that end, this patch set includes some new helper functions in
file_ptr
that enable efficient offset table pattern, with the added benefit of (I think?) solving the absolute/relative offset issue: now, a user puts the data field in the correct place in the struct and it Just Works, or else can use#[br(seek_before)]
on the data field if the offsets are relative to a different position in the file, which means the offset position can be relative or absolute and there is no need for special offset plumbing.I think this also suggests a path forward for offset table writing, where a temporary marker for the offset table can be created that zero-fills the space, and then the data field and the marker are passed to a helper function that writes the data and then writes out the offset table, but that isn’t something I’ve actually prototyped yet.