-
Notifications
You must be signed in to change notification settings - Fork 923
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
Remove HalSurface and fix layout assumptions in AnySurface and AnyDevice #5081
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.
I've done a slow pass over this, and the broad strokes LGTM. It's always good to avoid "We upgraded rustc
, and things broke, and it's our fault." 😅 I want to do another review pass focused on the fine-grained logic and types here, since my experience with Rust FFI is that there's often subtle bugs lurking in even the best human's code that aren't obvious. Will hopefully report back in the next day or so.
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.
praise: This is a nice simplification and fix. Thanks! 🙂
Overall, LGTM minus suggestions and nits! I'm marking as Request changes
so that the issues I've found get handled, but I'm confident in the overall thrust of this PR. On that note, I'm going to prompt for feedback from @Wumpf and/or @cwfitzgerald, since while I've done quite a bit of FFI/pointer-slinging in Rust before, I'm not as familiar with native platform domain/API for surfaces and devices.
I used Conventional Comments in this review! I hope they help with clarity and tone. 🙂
Thanks for taking a look @ErichDonGubler! I'll rebase the change once it has been looked over by all parties who might find it objectionable and the outstanding questions have been resolved. |
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.
LGTM now! Planning on waiting to merge this for a day or so, to give @jimblandy or @cwfitzgerald a chance to look over the pings I've given them specifically. I don't see a strong impetus to wait longer than that, though.
All right, I've rebased the change to try and get CI green so you're unblocked once you make a decision. I've also included this minor nit, because it doesn't make much sense to format one as a tuple and omit useful information in the other. |
6d3aa52
to
3977f34
Compare
Connections
#5080
Description
This avoids figuring out if #5080 is an actual problem by introducing an internal vtable for the types in question instead of using a trait object. This also affords us to stored the
backend
statically in the vtable, which hopefully makes figuring out which backend anAnySurface
belongs to much nicer.I've also reworked
AnySurface
to avoid a double-indirection, since AFAICT it only really needs to hold onto anA::Surface
. So it can be directly boxed.Checklist
cargo fmt
.cargo clippy
.cargo xtask test
to run tests.