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

automatically truncate larger integer types passed to bitfields #113

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

Lotterleben
Copy link
Contributor

closes #102

Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

Looks good! Left some comments

src/export.rs Show resolved Hide resolved
src/export.rs Outdated Show resolved Hide resolved
macros/src/lib.rs Outdated Show resolved Hide resolved
@@ -36,7 +36,7 @@ fn main() -> ! {
isize::min_value()
);
binfmt::info!("usize: 0 = {:usize}, MAX = {:usize}", 0, usize::max_value());
binfmt::info!("bitfields {0:0..7} {0:9..14}", 0b0110_0011_1101_0010u16);
binfmt::info!("bitfields {0:0..3} {0:5..7}", 0b0110_0011_1101_0110u16);
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

   --> /tmp/mdbook-FMDBPy/bitfields.md:25:1
    |
4   | binfmt::trace!("first two bits: {0:0..3}", 254);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `binfmt::export::sealed::Truncate<u8>` is not implemented for `i32`

this book test is failing due to type inference (as mentioned in the issue description). you'll need to specify the type of the literal, e.g. 254u8

src/export.rs Outdated
}

// needed so we can call truncate() without having to check whether truncation is necessary first
impl Truncate<u32> for u32 {
Copy link
Member

Choose a reason for hiding this comment

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

could we move this one further down? so that all Truncate<u16> impls are next to each other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done

@Lotterleben
Copy link
Contributor Author

Lotterleben commented Aug 13, 2020

   --> /tmp/mdbook-FMDBPy/bitfields.md:25:1
    |
4   | binfmt::trace!("first two bits: {0:0..3}", 254);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `binfmt::export::sealed::Truncate<u8>` is not implemented for `i32`

this book test is failing due to type inference (as mentioned in the issue description). you'll need to specify the type of the literal, e.g. 254u8

So are we happy with always having to specify the type for now or should this be fixed while we're at it? if the former, I'll add some doc pointing this out

@japaric
Copy link
Member

japaric commented Aug 13, 2020

So are we happy with always having to specify the type for now or should this be fixed while we're at it?

In practice most of the time you'll be passing something fully typed, e.g. my_register.read() which returns a u32 for example.
The type inference error happens when you use (untyped) literals as the argument and that scenario mainly occurs in tests and sample programs.
Would be good to document that a fully typed value is expected: the type must be one of u{8,16,32,64}

@japaric
Copy link
Member

japaric commented Aug 13, 2020

should this be fixed while we're at it?

addendum: we can "fix" this particular test by implementing Truncate<u8> for i32 but I think allowing signed integers makes the feature more confusing: what happens with the sign bit? is the bit 31 of i32 the sign bit? should bit 15 return the sign bit if the arg is i32 but then casted to u16? This feature is meant for viewing registers and those are not signed.

@jonas-schievink
Copy link
Contributor

Yeah I don't think this need to be fixed if it's documented right.

@Lotterleben
Copy link
Contributor Author

Lotterleben commented Aug 13, 2020

done!

@japaric japaric merged commit 124e3dc into main Aug 14, 2020
@jonas-schievink jonas-schievink deleted the bitfield_autotruncate branch October 19, 2020 15:57
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.

bitfield: accept larger integer types
3 participants