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

Allow deriving 'Trace' on 'Copy' types #88

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mystor
Copy link
Collaborator

@mystor mystor commented Apr 25, 2020

This change means that the Finalize::finalize method is not automatically called on drop. This might be a reasonable approach to take, and would avoid the need for a separate derive, but would be a breaking change.

Fixes #87

This change means that the `Finalize::finalize` method is not automatically
called on drop. This might be a reasonable approach to take, and would avoid the
need for a separate derive, but would be a breaking change.

Fixes #87
@Manishearth
Copy link
Owner

The diagnostics on this seem like they would be terrible :/

Plus, isn't this a behavior change? How is finalization handled after this?

@mystor
Copy link
Collaborator Author

mystor commented Apr 27, 2020

It is a behaviour change, as I mention in the first comment, which is why I just have this marked as a draft.

And yes, the diagnostics aren't amazing.

@Manishearth
Copy link
Owner

Yeah, I'm uncomfortable about behavior changes, we do have users (like boa) and behavior changes are far nastier than compile-breaking changes.

Tbh it would be nice if we could just blanket-impl trace for copy types, but that would preclude a bunch of other things

@Razican
Copy link
Contributor

Razican commented Apr 28, 2020

The issue I have seen with doing a blanked-impl is that then the compiler prevents implementing Trace for any non-Copy type in the standard library. (so no trace for String, Vec and so on.

A #[derive(TraceCopy)] would work too, I think.

@Manishearth
Copy link
Owner

yeah, that's the stuff the blanket impl would preclude 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot derive Trace for a Copy object.
4 participants