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

WIP - Solves #2000 to conditionally enable uniffi::constructor macro #2086

Closed
wants to merge 3 commits into from

Conversation

rafaelbeckel
Copy link
Contributor

The main discussion happened at #2000

This doesn't break any current test case. WIP because it should include new test cases covering some of the examples mentioned in the original discussion, but I'm still figuring out where they should go. I'll do that later this week.

Reviews are welcome.

The proc-macro method is now mentioned upfront, instead of as a bottom note. This provides clearer context for new users.
This PR solves mozilla#2000 - Unable to conditionally enable uniffi::constructor macro.
@rafaelbeckel rafaelbeckel requested a review from a team as a code owner May 1, 2024 00:35
@rafaelbeckel rafaelbeckel requested review from jeddai and removed request for a team May 1, 2024 00:36
Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

This doesn't do what you were talking about in #2000. This would make #[hello::world::uniffi::constructor] work, but not #[cfg_attr(something, uniffi::constructor)].

@rafaelbeckel
Copy link
Contributor Author

I see; thanks for pointing it out. I went at it with a wrong assumption. I'll get the tests working, they would have caught it earlier.

@bendk
Copy link
Contributor

bendk commented May 9, 2024

Have you checked out my comment in the issue? What do you think about the suggested approach? It's probably a bit more involved, but I can try to refactor some things so that it's not too bad.

@rafaelbeckel
Copy link
Contributor Author

Closing in favor of #2113

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.

3 participants