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

Added big endian feature for little endian CPU #697

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xtmono
Copy link

@xtmono xtmono commented Oct 10, 2022

Handle big endian data on little endian CPU.
Useful for dealing with network data.

Copy link
Contributor

@kkysen kkysen left a 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
endian: endian,
endian,

Comment on lines +31 to +35
let byte_index = if big_endian {
field.len() - 1 - (bit_index / 8)
} else {
bit_index / 8
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
};

Copy link
Contributor

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")
Copy link
Contributor

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?

Copy link
Contributor

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).

Comment on lines +235 to +243
let field_endians: Vec<_> = bitfields
.iter()
.map(|field| {
field
.endian
.as_ref()
.map_or(false, |endian| endian == "big")
})
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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<_>>();

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.

None yet

3 participants