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

Wrap unsafe function's bodies in unsafe blocks #2266

Merged
merged 4 commits into from
Nov 4, 2022
Merged

Wrap unsafe function's bodies in unsafe blocks #2266

merged 4 commits into from
Nov 4, 2022

Conversation

pvdrz
Copy link
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 emilio, kulp and amanjeev 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

☔ 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
28 tasks
@bors-servo
Copy link

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

src/lib.rs Outdated Show resolved Hide resolved
@pvdrz
Copy link
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
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
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
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

src/lib.rs Outdated Show resolved Hide resolved
@bors-servo
Copy link

☔ 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
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

☔ 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

☔ 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

☔ 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
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!

bindgen/lib.rs Outdated Show resolved Hide resolved
@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
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
Contributor

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

@pvdrz
Copy link
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
Contributor

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
Contributor Author

pvdrz commented Nov 18, 2022

#2354 should fix this

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

@amanjeev amanjeev amanjeev approved these changes

@kulp kulp Awaiting requested review from kulp

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