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

feat: allow deriving Trace without Drop/Finalize #116

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CertainLach
Copy link
Contributor

To allow structs with generated Trace impl to be destructured

Custom Drop implementation still can't be written, because generated
TriviallyDrop implementation tries to destruct value, which is only
possible when there is no user defined Drop impl

Empty Finalize impl generated too, because it will never be called

Signed-off-by: Yaroslav Bolyukin <[email protected]>
@Manishearth
Copy link
Owner

cc @kneecaw

gc/src/trace.rs Outdated Show resolved Hide resolved
@Manishearth
Copy link
Owner

cc @mystor

gc_derive/src/lib.rs Outdated Show resolved Hide resolved
gc_derive/src/lib.rs Outdated Show resolved Hide resolved
gc_derive/src/lib.rs Outdated Show resolved Hide resolved
gc_derive/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Yaroslav Bolyukin <[email protected]>
@CertainLach
Copy link
Contributor Author

Didn't meant to bother you with broken code, accidentally pushed, and then was busy, sorry.

CertainLach added a commit to CertainLach/jrsonnet that referenced this pull request Jun 5, 2021
Some manual Trace/Finalize implementations can be replaced with derives
with Manishearth/rust-gc#116 getting merged

Signed-off-by: Yaroslav Bolyukin <[email protected]>
CertainLach added a commit to CertainLach/jrsonnet that referenced this pull request Jun 5, 2021
Some manual Trace/Finalize implementations can be replaced with derives
with Manishearth/rust-gc#116 getting merged

Signed-off-by: Yaroslav Bolyukin <[email protected]>
Copy link
Owner

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Sorry about the delay! This looks good, happy to merge it in but would like some additional comments.

Ideally, it would be nice if this could be documented somewhere too.

#drop_impl
}
} else {
s.bind_with(|_| BindStyle::Move);
Copy link
Owner

Choose a reason for hiding this comment

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

Can we add some comments here as to why this branch is safe?

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.

3 participants