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.

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

Replace repr with with #172

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

csnover
Copy link
Collaborator

@csnover csnover commented Nov 16, 2022

Unfortunately, the repr feature does not work and fundamentally cannot work efficiently because the write-side always receives references to values, but (1) there are no From<&T> conversions for any std type that anyone would use as a repr, and (2) this would cause a complete copy of the converted object to be retained in memory just to write the object. So this PR removes repr entirely from the library and replaces it with something that definitely doesn’t have any of its own serious flaws or unnecessary complexity 👁️👄👁️.

The replacement feature, with, uses separate conversion types.

To use a conversion type, there are a few options: the attribute (e.g. #[brw(with(NullString))] field: String), the With wrapper (e.g. field: With<NullString, String>), or by calling the methods of the new ReadWith and WriteWith traits or the new methods on the existing BinReaderExt and BinWriterExt traits.

To implement a conversion type, implement ReadFrom<ConverterT> for T and WriteInto<ConverterT> for T. A converter can be used with many types; for example, in this patch, the NullString converter is implemented to read into Vec<u8> and String types, and to write from [u8], str, or String types. (n.b. Should that actually use a generic for anything that is like AsRef<[u8]>? Maybe, please bring it up in a code review!)

Since this creates a new idiom for how to serialise objects that don’t have obvious default serialisations (i.e. String), the NullString and NullWideString types are no longer containers, but rather just converters. Authors would switch to use an appropriate concrete type (Vec<u8> or String) and the with directive, or else the With wrapper class.

This patch doesn’t yet include any intermediate trait to enable third party to third party conversions as mentioned in #98. The number of different ways to do custom parsing is starting to feel a little burdensome; this seemed like a place to start.

@csnover csnover force-pushed the with-outwith-reprs branch 3 times, most recently from 2111381 to e2a91f7 Compare November 21, 2022 01:33
This feature does not work and fundamentally cannot work because
the write-side always receives references to values, but (1) there
are no `From<&T>` conversions for any std type that anyone would
use as a repr, and (2) this would cause a complete copy of the
converted object to be retained in memory just to write the object.
The `with` feature offers a more appropriate approach to custom
serialisations that does not suffer from these limitations.

This reverts:

"Implement top-level `repr` attribute for structs and enums."
commit 6f32003

"Fix nightly tests"
commit 38b48d4

"Implement field-level `repr` for conversion."
commit 8353699

...with the exception of portions of those commits which fixed a
bug where arbitrary errors from `try_map` in BinWrite were not
allowed due to overly-restrictive type hints.
@codecov-commenter
Copy link

Codecov Report

Merging #172 (ed89b93) into master (a9fda29) will decrease coverage by 0.21%.
The diff coverage is 0.00%.

❗ Current head ed89b93 differs from pull request most recent head 99696d4. Consider uploading reports for the commit 99696d4 to get more accurate results

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   95.53%   95.32%   -0.22%     
==========================================
  Files          64       64              
  Lines        6719     6734      +15     
==========================================
  Hits         6419     6419              
- Misses        300      315      +15     
Impacted Files Coverage Δ
binrw/src/io/no_std/mod.rs 56.72% <0.00%> (-3.82%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@csnover csnover marked this pull request as draft November 29, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants