-
Notifications
You must be signed in to change notification settings - Fork 715
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
Add failing regression test for opaque types in #1973 #1985
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok, though honestly passing opaque types by value is possibly broken in other cases, and I'm not sure how fixable / easy to fix it'd be, since it's obviously ABI-dependent.
Isn't handling opaque types by value the main reason for opaque types? Otherwise you could use an unsized type and only operate on pointers. |
Not really. At least when I implemented it the main use case was "I have this class/struct with this very complex type inside which I don't care about, but I care about the class having the right layout". Off hand I don't know how we'd go about fixing this (without ABI-specific knowledge), tbf. |
Ah, right. I don't really understand when you would require the correct layout for an opaque type other than to pass by value (or to allocate memory/copy memory/etc.). |
To access other fields? You can pass a pointer to something and poke at it from rust. |
Oh, sorry, I understand now. For when you have a struct with some fields as opaque and some not. |
☔ The latest upstream changes (presumably 2cfc64e) made this pull request unmergeable. Please resolve the merge conflicts. |
Let's close this for now since I don't think we want to codify this behavior. Passing opaque types by value is not guaranteed to work. |
The proposed fix #1984 changes the way unions are emitted, but the issue described in #1973 also affects homogenous aggregates emitted as opaque types. This pull request adds a similar integration test for that case.