binrw icon indicating copy to clipboard operation
binrw copied to clipboard

Assessment of attributes

Open jam1garner opened this issue 3 years ago • 7 comments

Tracking issue for figuring out how attributes will translate into the binread/binwrite merge. The main focus being which binread attributes should "just work", which can be accessible for both but not together, etc.

Key:

  • br - works for binread only
  • bw - works for binwrite only
  • br, bw - works for both, however a different value will need to be provided for each (example: map is a one way operation, so two maps will need to be provided to convert in both directions)
  • brw - works for both using a single implementation. One example could be "align", where a single alignment value would allow for both reading and writing. Would also allow for separate implementations of br and bw if desired.

binread attributes

attribute compatibility more info
big brw
little brw
map br, bw
magic brw
assert br, bw up for debate, usefulness of brw questionable
pre_assert br, bw up for debate, usefulness of brw questionable
import br, bw up for debate, usefulness of brw questionable
args br, bw
default br possibly should be removed in favor of ignore
ignore brw
temp br, bw
postprocess_now br dependent on the design of the binwrite redesign, this may have some usability
deref_now br ditto
restore_position br, bw
try br binwrite likely has no use as the reverse should be the default behavior for Option<T> where T: BinWrite
parse_with br
calc br, bw
is_little brw
is_big brw
offset br, bw(?) needs discussion, not sure how this will work
if br should "just work" since it reads to an Option<T>
pad_before brw
pad_after brw
align_before brw
align_after brw
seek_before brw
pad_size_to brw
return_all_errors br(?) error handling probably needs a rework tbh

binwrite attributes

attribute compatibility more info
preprocessor no superseded by map
postprocessor ??? probably needs a rework
with bw rename to write_with, name pending
cstr brw a useful shortcut that probably should've carried over to binread
utf16 brw ditto
utf16_null brw ditto
align no renamed to align_before
pad no renamed to pad_before

jam1garner avatar Nov 01 '20 20:11 jam1garner

Ideas for attributes to add/replace:

pre_assert is a bit of a hacky workaround to allow arbitrary placement of magics. It won't work nicely for a binwrite variant. Ideally, it should get replaced with something like magic_for, e.g.:

enum MyEnum {
    #[br(magic = "1u32")] X { /* ... */ },
    #[br(magic = "2u32")] Y { /* ... */ },
}

struct X {
    #[brw(magic_for = "data")]
    magic: u32,
    len: u32,
    data: MyEnum
}

If a magic_for attribute is present, the enum should not read the magic itself. Instead, the deserializer for X will provide it himself.

roblabla avatar Nov 01 '20 20:11 roblabla

Another addition: #[br(dbg)]

is essentially equivalent to #[br(map = |x| dbg!(x))] for printing out values mid-parse in order to allow for debugging when parsing fails.

jam1garner avatar Nov 01 '20 20:11 jam1garner

possible suggestion for brw attributes: br,bw versions of them so that if (for some reason) you can override them seperately if needed.

tbh this is probably has dubious merit.

kitlith avatar Nov 01 '20 20:11 kitlith

possible suggestion for brw attributes: br,bw versions of them so that if (for some reason) you can override them seperately if needed.

tbh this is probably has dubious merit.

Yep @kitlith, that's exactly what I meant by the following part of the key:

Would also allow for separate implementations of br and bw if desired.

jam1garner avatar Nov 01 '20 20:11 jam1garner

Ah, i missed that.

kitlith avatar Nov 01 '20 20:11 kitlith

Another addition idea: #[br(take = $expr)], equivalent to "taking" $expr bytes. Which is to say, a reader adapter for limiting it to $expr bytes. Which would pretty much just involve temporarily replacing the reader with https://doc.rust-lang.org/std/io/trait.Read.html#method.take

jam1garner avatar Nov 06 '20 07:11 jam1garner

@jam1garner take as an attribute is a good idea that I've found myself reaching for in the past, but it's not quite that simple as the standard take adapter doesn't implement Seek (so iirc we'd run into trait constraint issues), and we probably want to allow FilePtr to (optionally?) read outside the taken area. The latter, in particular, probably causes the most issues with an adapter based approach?

kitlith avatar Nov 08 '20 02:11 kitlith