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.

Sign up for GitHub

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

Jump to bottom

newruntime: implement reset #11206

Closed
wants to merge 2 commits into from
Closed

newruntime: implement reset #11206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea467f9
reset for newruntime
cooldome May 8, 2019
3ca57ae
improve test
cooldome May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions lib/system.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,11 @@ proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {.
## **Note**: The `finalizer` refers to the type `T`, not to the object!
## This means that for each object of type `T` the finalizer will be called!

proc reset*[T](obj: var T) {.magic: "Reset", noSideEffect.}
## Resets an object `obj` to its initial (binary zero) value.
##
## This needs to be called before any possible `object branch transition`:idx:.
when not defined(nimV2):
proc reset*[T](obj: var T) {.magic: "Reset", noSideEffect.}
## Resets an object `obj` to its initial (binary zero) value.
##
## This needs to be called before any possible `object branch transition`:idx:.

proc wasMoved*[T](obj: var T) {.magic: "WasMoved", noSideEffect.} =
## Resets an object `obj` to its initial (binary zero) value to signify
Expand Down Expand Up @@ -4441,6 +4442,14 @@ when defined(genode):
# and return to thread entrypoint.


when defined(nimV2):
proc reset*[T](obj: var T) =
## Resets an object `obj` to its initial (binary zero) value.
## 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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.


import system/widestrs
export widestrs

Expand Down
19 changes: 16 additions & 3 deletions tests/destructor/tnewruntime_strutils.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
discard """
cmd: '''nim c --newruntime $file'''
output: '''442 442'''
output: '''442 442
443 443
'''
"""

import strutils, os
Expand Down Expand Up @@ -187,5 +189,16 @@ proc staticTests =
nonStaticTests()
staticTests()

let (a, d) = allocCounters()
discard cprintf("%ld %ld\n", a, d)
block:
let (a, d) = allocCounters()
discard cprintf("%ld %ld\n", a, d)

var x = newSeq[string](2)
reset(x)

var x2 = 2
reset(x2)

block:
let (a, d) = allocCounters()
discard cprintf("%ld %ld\n", a, d)