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.

Sign up for GitHub

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

Jump to bottom

Sign extend signed integers #238

Merged
merged 3 commits into from
Feb 25, 2022
Merged

Sign extend signed integers #238

merged 3 commits into from
Feb 25, 2022

Conversation

korrat
Copy link
Contributor

@korrat korrat commented Oct 16, 2021

This PR changes the implementation of DekuRead for signed integer primitive types, so that reads with a bit size less than the size of the underlying type are sign extended.

Fixes #236.

@wcampbell0x2a
Copy link
Collaborator

Could you add tests to the tests directory? Just for proving these fields in structs and such.

@sharksforarms
Copy link
Owner

Could you add tests to the tests directory? Just for proving these fields in structs and such.

Since there's no change to attributes I think it's alright as is (current test cases were updated)

@sharksforarms
Copy link
Owner

Is there a way to reduce the duplication of code between signed/non-signed? I'm open to having this as a separate issue.

@sharksforarms
Copy link
Owner

Friendly ping @korrat are you still interested in pursuing this PR?

@korrat korrat force-pushed the sign-extend-signed-integers branch from 4d118dc to 547d83c Compare January 23, 2022 07:52
@korrat
Copy link
Contributor Author

korrat commented Jan 23, 2022

Sorry, @sharksforarms, totally forgot about this one. I'm still interested in pursuing this PR.

Is there a way to reduce the duplication of code between signed/non-signed? I'm open to having this as a separate issue.

I've tried to merge the ImplDekuRead macros, but I'm having some trouble with the f32/f64, since they don't implement Shl/Shr. I can try again later in the week, but I think I would have to add a new macro for the floats. But perhaps that macro could be simpler.

@korrat
refactor: reduce duplication in DekuRead macros
9e3b152 
Previously, the two macros to implement DekuRead for integer primitives
contained nearly identical code, with the exception of sign-extending
in the signed integer implementation.

With this commit, signed integers reuse the code of their unsigned
counterparts and add sign-extending on top.
@korrat
Copy link
Contributor Author

korrat commented Feb 15, 2022

Here's a proposal to reduce the duplication. Let me know if this is what you had in mind.

Alternatively, it should be possible to use a single implementation for the integer types and instead proxy the float implementations through unsigned integers, similarly to how it's done for the signed integers in this commit.

@korrat
Copy link
Contributor Author

korrat commented Feb 22, 2022

I can reproduce the compile error on master, so I think it is unrelated to the changes in this PR.

I think the error is related to rust-lang/rust#79609. Defining _Unwind_Resume, as suggested there, fixes the problem. Should I add a commit for the fix? Or would you prefer a different PR for that?

@sharksforarms
Copy link
Owner

Hey @korrat sorry for my delay. The PR looks good at first glance, I'll take some time this week to look at it. A separate PR would be appreciated!

@sharksforarms sharksforarms merged commit f9013b8 into sharksforarms:master Feb 25, 2022
@sharksforarms
Copy link
Owner

Thanks! Released in 0.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharksforarms sharksforarms sharksforarms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sign-extending signed integer types?
3 participants
@korrat @wcampbell0x2a @sharksforarms