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

Use unsafe blocks in unsafe fns #1191

Closed
chbaker0 opened this issue Mar 8, 2023 · 6 comments · Fixed by #1281
Closed

Use unsafe blocks in unsafe fns #1191

chbaker0 opened this issue Mar 8, 2023 · 6 comments · Fixed by #1281

Comments

@chbaker0
Copy link

chbaker0 commented Mar 8, 2023

cxx::bridge generates code incompatible with the unsafe_op_in_unsafe_fn lint. Since this is a useful lint and it's likely to become an error in the future, cxx::bridge should wrap unsafe ops in unsafe blocks always.

Right now it's impossible to use it with forbid(unsafe_op_in_unsafe_fn) and requires explicit allow() attributes with deny(...)

@schreter
Copy link
Contributor

+1

We had some issues with this, too, and have fixed it in our project-private fork of the crate. Would be nicer to have it fixed upstream.

@bridiver
Copy link

+1

We had some issues with this, too, and have fixed it in our project-private fork of the crate. Would be nicer to have it fixed upstream.

@schreter why don't you submit the change as a PR?

@schreter
Copy link
Contributor

@bridiver

why don't you submit the change as a PR?

Bluntly put, because I feel it's pointless.

We wanted to contribute major stuff upstream (by-value passing, proper customizable exception support, etc.), primarily what we need for our "big" project with C++/Rust interop (about 8M LOCs C++ + 2M LOCs Rust). I have some PRs there open since half a year with no review from the maintainer, also no reaction to nudges in other PRs/issues.

Therefore, I gave up.

Maybe a small change like this would work, I got some micro-changes merged. Feel free to try to open a PR, but other small changes got also zero reaction from the maintainer. Anything what is not a trivial bugfix seems to be ignored.

I'm sorry for the project, but I can't expend the time for upstream anymore. Maybe we'll publish a new, divergent crate based on this one, which contains all our improvements, but I don't promise. No manpower to drive this and deadlines are pressing.

@antonok-edm
Copy link

Maybe we'll publish a new, divergent crate based on this one, which contains all our improvements, but I don't promise. No manpower to drive this and deadlines are pressing.

@schreter I highly recommend at least pushing your forked branch to GitHub. Judging from reactions on your other PRs, others are definitely interested in the changes. Some of us may be willing to take on the effort of ongoing maintenance, whether that means publishing a fork to crates.io or preparing and managing PRs for this repo.

Speaking from experience as a maintainer of a few open source projects, it takes a lot of activation energy to review PRs. @dtolnay maintains a lot of popular crates; I imagine he'll get to the PRs you've opened when he's ready.

@chbaker0
Copy link
Author

@dtolnay can you advise what's the best way to run the cxx test suite so that cxx::bridge macro output is built with forbid(unsafe_op_in_unsafe_fn)? I guess specifically for the trybuild tests.

Not looking for any further attention here, this just might help with writing a PR later.

@bridiver
Copy link

bridiver commented Aug 23, 2023

@chbaker0 we use -Dunsafe_op_in_unsafe_fn and we're doing this to work around it for cxx

#[allow(unsafe_op_in_unsafe_fn)]
#[cxx::bridge(...)]
...

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 a pull request may close this issue.

4 participants