Skip to content

Wrap unsafe function's bodies in unsafe blocks#2266

Merged
pvdrz merged 4 commits intorust-lang:masterfrom
ferrous-systems:unsafe-blocks-in-unsafe-fn
Nov 4, 2022
Merged

Wrap unsafe function's bodies in unsafe blocks#2266
pvdrz merged 4 commits intorust-lang:masterfrom
ferrous-systems:unsafe-blocks-in-unsafe-fn

Conversation

@pvdrz
Copy link
Copy Markdown
Contributor

@pvdrz pvdrz commented Sep 6, 2022 •
edited
Loading

This PR adds a new option --unsafe-blocks which wraps all the bodies of generated unsafe functions in unsafe blocks.

Fixes: #2063
Blocked by: #2265

@pvdrz pvdrz requested review from amanjeev, emilio and kulp and removed request for emilio and kulp September 6, 2022 21:08
@pvdrz pvdrz force-pushed the unsafe-blocks-in-unsafe-fn branch from 79e1051 to 953ca92 Compare September 9, 2022 21:20
@bors-servo
Copy link
Copy Markdown

bors-servo commented Sep 10, 2022

☔ The latest upstream changes (presumably 8b29355) made this pull request unmergeable. Please resolve the merge conflicts.

@pvdrz pvdrz force-pushed the unsafe-blocks-in-unsafe-fn branch from 953ca92 to d4ba612 Compare September 10, 2022 01:26
@pvdrz pvdrz marked this pull request as ready for review September 10, 2022 01:26
@pvdrz pvdrz mentioned this pull request Sep 15, 2022
Open
45 tasks
@bors-servo
Copy link
Copy Markdown

bors-servo commented Sep 23, 2022

☔ The latest upstream changes (presumably 4b006da) made this pull request unmergeable. Please resolve the merge conflicts.

emilio
emilio approved these changes Sep 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@emilio emilio 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 code looks good, after a rebase. I'm not quite sure about the feature, in the sense that it doesn't add any real benefit over saving the user typing #[allow(..)] for a feature they need to opt-into anyways.

I think it'd be good to move all these post-processing operations to their own file since putting it in lib.rs feels a bit odd and creates a lot of rightward drift. But that can be a follow-up I suppose.

Comment thread src/lib.rs Outdated
@pvdrz
Copy link
Copy Markdown
Contributor Author

pvdrz commented Sep 23, 2022 •
edited
Loading

I implemented this because I thought you agreed to it 😬

That being said, I think it is not unheard of bindgen having features that could be fixed by one liners or --raw-lines here and there.

cc @ojeda just because they requested the feature and might want to give their opinion

@pvdrz pvdrz force-pushed the unsafe-blocks-in-unsafe-fn branch 2 times, most recently from aca0a08 to 225eeca Compare September 23, 2022 17:37
@ojeda
Copy link
Copy Markdown
Contributor

ojeda commented Sep 23, 2022 •
edited
Loading

It is definitely not critical, but it allows a project to call the compiler with a global -Funsafe_op_in_unsafe_fn flag, for instance.

Moreover, considering that the language is evolving towards not implying unsafe blocks in unsafe fns (the current plan seems to be that Edition 2024 warns-by-default), it seems like a good idea to make bindgen ready to generate idiomatic code (and do so by default when targeting the future edition when that arrives).

@pvdrz
Copy link
Copy Markdown
Contributor Author

pvdrz commented Sep 23, 2022

I think it'd be good to move all these post-processing operations to their own file since putting it in lib.rs feels a bit odd and creates a lot of rightward drift. But that can be a follow-up I suppose.

@emilio check #2282

amanjeev
amanjeev approved these changes Sep 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@amanjeev amanjeev left a comment

Choose a reason for hiding this comment

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

Thank you

Comment thread src/lib.rs Outdated
@bors-servo
Copy link
Copy Markdown

bors-servo commented Sep 27, 2022

☔ The latest upstream changes (presumably #2282) made this pull request unmergeable. Please resolve the merge conflicts.

@pvdrz pvdrz force-pushed the unsafe-blocks-in-unsafe-fn branch 2 times, most recently from 66be332 to 07d6134 Compare September 27, 2022 19:40
@pvdrz
Copy link
Copy Markdown
Contributor Author

pvdrz commented Oct 4, 2022

@emilio you approved this PR but from your comments it is not clear to me that you want this actually merged.

@bors-servo
Copy link
Copy Markdown

bors-servo commented Oct 5, 2022

☔ The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts.

@pvdrz pvdrz force-pushed the unsafe-blocks-in-unsafe-fn branch from 07d6134 to e2d471f Compare October 5, 2022 02:17
@bors-servo
Copy link
Copy Markdown

bors-servo commented Nov 2, 2022

☔ The latest upstream changes (presumably 9c32b46) made this pull request unmergeable. Please resolve the merge conflicts.

@pvdrz pvdrz force-pushed the unsafe-blocks-in-unsafe-fn branch 2 times, most recently from e3cf0d7 to dfd69e2 Compare November 4, 2022 15:44
@bors-servo
Copy link
Copy Markdown

bors-servo commented Nov 4, 2022

☔ The latest upstream changes (presumably bae6170) made this pull request unmergeable. Please resolve the merge conflicts.

emilio
emilio approved these changes Nov 4, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Assuming there are no warnings for redundant unsafe blocks etc this looks good!

Comment thread bindgen/lib.rs Outdated
@pvdrz pvdrz force-pushed the unsafe-blocks-in-unsafe-fn branch from dfd69e2 to eadaa39 Compare November 4, 2022 18:20
@pvdrz pvdrz merged commit e8ffb42 into rust-lang:master Nov 4, 2022
@pvdrz pvdrz deleted the unsafe-blocks-in-unsafe-fn branch November 4, 2022 20:22
@lopopolo
Copy link
Copy Markdown
Contributor

lopopolo commented Nov 4, 2022

@pvdrz you've been on a roll recently! Thanks for pushing through so many tickets I've been subscribed to!

@glandium
Copy link
Copy Markdown
Contributor

glandium commented Nov 17, 2022

Unfortunately, this broke building with -Dwarnings on rustc < 1.65.

@pvdrz
Copy link
Copy Markdown
Contributor Author

pvdrz commented Nov 17, 2022

Unfortunately, this broke building with -Dwarnings on rustc < 1.65.

Do you have any reduced case so we can add it to the tests?

@glandium
Copy link
Copy Markdown
Contributor

glandium commented Nov 17, 2022

Any of the test files that were touched in e8ffb42 shows the warning with rustc < 1.65.

pvdrz added a commit to ferrous-systems/rust-bindgen that referenced this pull request Nov 18, 2022
@pvdrz
Revert "Wrap unsafe function's bodies in unsafe blocks (rust-lang…
ea99875 
…#2266)"

This reverts commit e8ffb42.
@pvdrz
Copy link
Copy Markdown
Contributor Author

pvdrz commented Nov 18, 2022

#2354 should fix this

@OmriSteiner OmriSteiner mentioned this pull request Feb 28, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emilio emilio emilio approved these changes

@kulp kulp Awaiting requested review from kulp

+1 more reviewer

@amanjeev amanjeev amanjeev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support unsafe_op_in_unsafe_fn

7 participants

@pvdrz @bors-servo @ojeda @lopopolo @glandium @amanjeev @emilio