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

Don't box value in Fragile<T> #28

Merged
merged 1 commit into from Oct 18, 2022
Merged

Conversation

Nilstrieb
Copy link
Contributor

I'm not sure why the boxing was done here, but it isn't required for soundness and not doing it is more efficient so I've removed it. Maybe there's a good reason for doing it that I haven't considered.

@mitsuhiko
Copy link
Owner

I'm in favor of this change. I wanted to do this change before but I wasn't too comfortable with the behavior of SemiSticky. It's unclear if that type even makes too much sense, but if that change does happen it probably should Box there for the fragile case.

@mitsuhiko
Copy link
Owner

I'm actually not entirely sure about the change now. Today Fragile<T> boxes internally and in turn accomplishes that Drop is a noop for a lot of types. In the future Fragile<Box<T>> would invoke the dtor even in cases where it's not necessary because mem::needs_drop::<Box<T>>() would always return true. This severely limits the usefulness of Fragile as all drops in another thread for boxed values would abort the process.

@Nilstrieb
Copy link
Contributor Author

People can just use Box<Fragile<T>> couldn't they?

@mitsuhiko
Copy link
Owner

mitsuhiko commented Oct 18, 2022

I'm actually wondering now if this does not mean there is a memory leak in Fragile now for values without dtor. I need to validate this. You are probably right that one can just box the entire fragile instead.

@mitsuhiko mitsuhiko merged commit 0a99af0 into mitsuhiko:master Oct 18, 2022
@Nilstrieb Nilstrieb deleted the nobox branch October 18, 2022 09:33
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.

None yet

2 participants