jam1garner / binrw

A Rust crate for helping parse and rebuild binary data using ✨macro magic✨.
https://binrw.rs
MIT License
585 stars 35 forks source link

Replace `repr` with `with` #172

Open csnover opened 1 year ago

csnover commented 1 year ago

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.

codecov-commenter commented 1 year ago

Codecov Report

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

:exclamation: 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%) :arrow_down:

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