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

[Destructors] Wrong moves and copies #9294

Closed
b3liever opened this issue Oct 10, 2018 · 1 comment

Comments

Projects
None yet
3 participants
@b3liever
Copy link
Contributor

commented Oct 10, 2018

After 0803b53 there seems
that sometimes copies can be prevented while elsewhere they should be made.
Most obvious example is this:

proc main =
   var a = matrix(5, 5, 1.0)
   a = -a + a

Doesn't make a copy.

type
  Matrix* = object
    # Array for internal storage of elements.
    data: ptr UncheckedArray[float]
    # Row and column dimensions.
    m*, n*: int

var
  allocCount, deallocCount: int

proc `=destroy`*(m: var Matrix) =
  if m.data != nil:
    dealloc(m.data)
    deallocCount.inc
    m.data = nil
    m.m = 0
    m.n = 0

proc `=sink`*(a: var Matrix; b: Matrix) =
  if a.data != nil and a.data != b.data:
    dealloc(a.data)
    deallocCount.inc
  a.data = b.data
  a.m = b.m
  a.n = b.n

proc `=`*(a: var Matrix; b: Matrix) =
  if a.data != nil and a.data != b.data:
    dealloc(a.data)
    deallocCount.inc
    a.data = nil
  a.m = b.m
  a.n = b.n
  if b.data != nil:
    a.data = cast[type(a.data)](alloc(a.m * a.n * sizeof(float)))
    allocCount.inc
    copyMem(a.data, b.data, b.m * b.n * sizeof(float))

proc matrix*(m, n: int): Matrix =
  ## Construct an m-by-n matrix of zeros. 
  result.m = m
  result.n = n
  result.data = cast[type(result.data)](alloc(m * n * sizeof(float)))
  allocCount.inc

proc matrix*(m, n: int, s: float): Matrix =
  ## Construct an m-by-n constant matrix.
  result.m = m
  result.n = n
  result.data = cast[type(result.data)](alloc(m * n * sizeof(float)))
  allocCount.inc
  for i in 0 ..< m * n:
    result.data[i] = s

proc `[]`*(m: Matrix, i, j: int): float {.inline.} =
  ## Get a single element.
  m.data[i * m.n + j]

proc `[]`*(m: var Matrix, i, j: int): var float {.inline.} =
  ## Get a single element.
  m.data[i * m.n + j]

proc `[]=`*(m: var Matrix, i, j: int, s: float) =
  ## Set a single element.
  m.data[i * m.n + j] = s

proc `-`*(m: sink Matrix): Matrix =
  ## Unary minus
  result = m
  for i in 0 ..< m.m:
    for j in 0 ..< m.n:
      result[i, j] = -m[i, j]

proc `+`*(a: sink Matrix, b: Matrix): Matrix =
  ## ``C = A + B``
  assert(b.m == a.m and b.n == a.n, "Matrix dimensions must agree.")
  result = a
  for i in 0 ..< a.m:
    for j in 0 ..< a.n:
      result[i, j] = a[i, j] + b[i, j]

proc `+=`*(a: var Matrix, b: Matrix) =
  ## ``A = A + B``
  assert(b.m == a.m and b.n == a.n, "Matrix dimensions must agree.")
  for i in 0 ..< a.m:
    for j in 0 ..< a.n:
      a[i, j] = a[i, j] + b[i, j]

proc `-`*(a: sink Matrix, b: Matrix): Matrix =
  ## ``C = A - B``
  assert(b.m == a.m and b.n == a.n, "Matrix dimensions must agree.")
  result = a
  for i in 0 ..< a.m:
    for j in 0 ..< a.n:
      result[i, j] = a[i, j] - b[i, j]

proc `-=`*(a: var Matrix, b: Matrix) =
  ## ``A = A - B``
  assert(b.m == a.m and b.n == a.n, "Matrix dimensions must agree.")
  for i in 0 ..< a.m:
    for j in 0 ..< a.n:
      a[i, j] = a[i, j] - b[i, j]

template check(part1, part2): untyped =
  part1()
  let a = allocCount
  let d = deallocCount
  allocCount = 0
  deallocCount = 0
  part2()
  assert a == allocCount
  assert d == deallocCount
  allocCount = 0
  deallocCount = 0

# 0
proc test0p1 =
  var a = matrix(5, 5, 1.0)
  a = -a + a

proc test0p2 =
  var a = matrix(5, 5, 1.0)
  a -= a

# 1
proc test1p1 =
  var a = matrix(5, 5, 1.0)
  a = -a

proc test1p2 =
  var a = matrix(5, 5, 1.0)
  var b = -a

# 2
proc test2p1 =
  var a = matrix(5, 5, 1.0)
  var b = matrix(5, 5, 2.0)
  a = a + b

proc test2p2 =
  var a = matrix(5, 5, 1.0)
  var b = matrix(5, 5, 2.0)
  a += b

check(test0p1, test0p2)
check(test1p1, test1p2)
check(test2p1, test2p2)

@b3liever b3liever changed the title [Destructos] wrong moves and copies [Destructors] Wrong moves and copies Oct 10, 2018

@Araq

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

Btw, code like

proc `-`*(m: sink Matrix): Matrix =
  ## Unary minus
  result = m
  for i in 0 ..< m.m:
    for j in 0 ..< m.n:
      result[i, j] = -m[i, j]

should be written as

proc `-`*(m: sink Matrix): Matrix =
  ## Unary minus
  result = m
  for i in 0 ..< result.m:
    for j in 0 ..< result.n:
      result[i, j] = -result[i, j]

Since you moved the m already...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.