Skip to content

src/cdef_apply.rs: more cleanup#738

Merged
kkysen merged 13 commits into
mainfrom
cdef-apply-cleanup2
Feb 12, 2024
Merged

src/cdef_apply.rs: more cleanup#738
kkysen merged 13 commits into
mainfrom
cdef-apply-cleanup2

Conversation

@folkertdev
Copy link
Copy Markdown
Collaborator

builds on #733

Copy link
Copy Markdown
Collaborator Author

@folkertdev folkertdev left a comment

Choose a reason for hiding this comment

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

I think this is about as far as the cleanup of this file will go at the moment. More can be done when data structure fields become safe, but then cleaning up the code here should follow naturally from those changes.

Comment thread src/cdef_apply.rs
Comment on lines -115 to +114
slice::from_raw_parts(&mut *src[0].offset(y_off + x_off as isize - 2), 2),
slice::from_raw_parts(src[0].offset(y_off + x_off as isize - 2), 2),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

is that &mut something that c2rust inserts? it's not really needed and also unsound when that pointer is dangling (a dangling pointer would be sound if the slice length were 0). Not a problem in practice here but in general it seems odd to me.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Somewhat (it also looks extra weird since we didn't delete it as we partially cleaned it up/bitdepth deduplicated it). c2rust transpile sometimes has redundant &mut *s, and it also generates that kind of code when it really should generate addr_of_mut! to be sound (see immunant/c2rust#301).

Comment thread src/cdef_apply.rs
Base automatically changed from cdef-apply-cleanup to main February 12, 2024 22:38
@kkysen kkysen merged commit 66eeba1 into main Feb 12, 2024
@kkysen kkysen deleted the cdef-apply-cleanup2 branch February 12, 2024 22:39
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.

2 participants