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

newruntime: implement reset #11206

Closed
wants to merge 2 commits into from

Conversation

cooldome
Copy link
Member

@cooldome cooldome commented May 8, 2019

reset proc no longer needs to be magic

Copy link
Collaborator

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

At first glance this seems great but everytime we need mixin for a generic proc, I feel like we're opening a can of worms. See comment for details and link to test cases I feel are needed.

## Destructor is invoked if T type has destructor.
mixin `=destroy`
`=destroy`(obj)
zeroMem(obj.addr, sizeof(obj))
Copy link
Collaborator

@mratsim mratsim May 9, 2019

Choose a reason for hiding this comment

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

Can we add tests on non-seq generic objects in a generic proc and also normal object in a block expression.

Requiring mixin here might require every proc that would use reset to also have a mixin =destroy.
See:

Meta-issue: #8677 (Generics early symbol resolution)

Copy link
Member

Choose a reason for hiding this comment

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

Requiring mixin here might require every proc that would use reset to also have a mixin =destroy.

That's not how mixin works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In generics case it sometimes just does not work, here is another example i'm struggling with: status-im/nim-serialization#4

And the problematic type is a mixin in the template: https://github.com/status-im/nim-serialization/blob/6804ea25372de5919ef87cf13c7544770aacba2f/serialization.nim#L73

Copy link
Member Author

@cooldome cooldome May 9, 2019

Choose a reason for hiding this comment

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

I don't think mixin comments are relevant . =destroy invocation for generic type needs a mixin. Period. There is nothing reset specific here.
I have extended the test to cover a type with no destructor.

Copy link
Collaborator

@mratsim mratsim May 10, 2019

Choose a reason for hiding this comment

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

What I'm saying is that if we have symbol visibility rules that requires mixin for this proc, we may have symbol visibility issues where the mixin within reset doesn't work and the package author will have to mixin =destroy in his own proc.

@Araq
Copy link
Member

Araq commented May 9, 2019

It still needs to be a magic for the VM, I think, otherwise a nice solution.

@Araq
Copy link
Member

Araq commented Jul 10, 2019

I simply mapped it to =destroy in devel.

@Araq Araq closed this Jul 10, 2019
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