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

Rewrite Views to be Miri compatible #588

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

ibraheemdev
Copy link
Contributor

Rewrite the Views type to use a manual VTable instead of fat pointer manipulation. I find this pattern to be much clearer than the existing code.

The current implementation is also aliasing Box through the OpaqueBoxDyn and Box<dyn Free> pointers, which is dubious, although Miri seemed to be pleased with the small diff:

- type OpaqueBoxDyn = [u8; std::mem::size_of::<Box<dyn CastTo<Dummy>>>()];
+ type OpaqueBoxDyn = [MaybeUninit<u8>; std::mem::size_of::<Box<dyn CastTo<Dummy>>>()];

Copy link

netlify bot commented Oct 3, 2024

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit bd82274
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/6706b5c64af6070008668ad4

Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

I gave this a quick look and it seems reasonable. I know I considered this approach and decided against it; I'm trying to remember why. I'll look it over more closely later today.

Copy link

codspeed-hq bot commented Oct 6, 2024

CodSpeed Performance Report

Merging #588 will not alter performance

Comparing ibraheemdev:views-vtable (bd82274) with master (a20b894)

Summary

✅ 8 untouched benchmarks

Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Read it over again. I like it! Nice work.

@nikomatsakis
Copy link
Member

I'd merge this but clippy is grouchy.

@MichaReiser
Copy link
Contributor

I'll fix the clippy issue and merge the PR afterward.

@MichaReiser MichaReiser added this pull request to the merge queue Oct 9, 2024
Merged via the queue into salsa-rs:master with commit af2ec49 Oct 9, 2024
10 checks passed
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