-
Notifications
You must be signed in to change notification settings - Fork 249
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
Added big endian feature for little endian CPU #697
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test showing how (and testing that) this works and would be used? You can put them alongside the other bitfields tests in tests/
. Also, can you add documentation somewhere detailing how this can be used? Thanks!
@@ -97,6 +100,7 @@ fn parse_bitfield_attr( | |||
name: name.unwrap(), | |||
ty: ty.unwrap(), | |||
bits: (bits.unwrap(), bits_span.unwrap()), | |||
endian: endian, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endian: endian, | |
endian, |
let byte_index = if big_endian { | ||
field.len() - 1 - (bit_index / 8) | ||
} else { | ||
bit_index / 8 | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let byte_index = if big_endian { | |
field.len() - 1 - (bit_index / 8) | |
} else { | |
bit_index / 8 | |
}; | |
let byte_index = bit_index / 8; | |
let byte_index = if big_endian { | |
field.len() - 1 - byte_index | |
} else { | |
byte_index | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you refactor all 3 (the 2 below changes, too) of these same computations into some helper function?
field | ||
.endian | ||
.as_ref() | ||
.map_or(false, |endian| endian == "big") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it only check for big
or should it have to be big
, little
, or nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endian could be an enum, and if it had a default, this could be .unwrap_or_default()
. (I lack the context here to understand whether a default endianness makes sense at all; gut says no).
let field_endians: Vec<_> = bitfields | ||
.iter() | ||
.map(|field| { | ||
field | ||
.endian | ||
.as_ref() | ||
.map_or(false, |endian| endian == "big") | ||
}) | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let field_endians: Vec<_> = bitfields | |
.iter() | |
.map(|field| { | |
field | |
.endian | |
.as_ref() | |
.map_or(false, |endian| endian == "big") | |
}) | |
.collect(); | |
let field_endians = bitfields | |
.iter() | |
.map(|field| { | |
field | |
.endian | |
.as_ref() | |
.map_or(false, |endian| endian == "big") | |
}) | |
.collect::<Vec<_>>(); |
Handle big endian data on little endian CPU.
Useful for dealing with network data.