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

Assessment of attributes #3

Closed
jam1garner opened this issue Nov 1, 2020 · 7 comments
Closed

Assessment of attributes #3

jam1garner opened this issue Nov 1, 2020 · 7 comments

Comments

@jam1garner
Copy link
Owner

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
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
@roblabla
Copy link
Collaborator

roblabla commented Nov 1, 2020

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.

@jam1garner
Copy link
Owner Author

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.

@kitlith

This comment has been minimized.

@jam1garner
Copy link
Owner Author

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.

@kitlith
Copy link
Collaborator

kitlith commented Nov 1, 2020

Ah, i missed that.

@jam1garner
Copy link
Owner Author

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

@kitlith
Copy link
Collaborator

kitlith commented Nov 8, 2020

@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?

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

No branches or pull requests

3 participants