nim-cppstl icon indicating copy to clipboard operation
nim-cppstl copied to clipboard

doesn't work with destructors

Open timotheecour opened this issue 3 years ago • 2 comments

continuing of https://github.com/kaushalmodi/std_vector/issues/4 since the other repo was merged here

example

when defined case2:
  import cppstl/std_vector
  type Foo = object
    x: int
  proc `=destroy`*(a: var Foo) {.inline.} =
    echo ("in destroy", a.x)
  proc main =
    var v1 = initCppVector[Foo]()
    echo v1
    var v = initCppVector[Foo]()
    v.add Foo(x: 10)
    v.add Foo(x: 11)
    v.add Foo(x: 12)
    echo "ok0"
    echo v
    echo "ok1"
    echo v
    echo "ok2"
  main()

gives:

[]
ok0
("in destroy", 10)
("in destroy", 11)
("in destroy", 12)
[(x: 10), (x: 11), (x: 12)]
ok1
("in destroy", 10)
("in destroy", 11)
("in destroy", 12)
[(x: 10), (x: 11), (x: 12)]
ok2

(with both gc:arc|refc)

expected result

same as for here, with seq instead of vector:

when defined case3:
  import cppstl/std_vector
  type Foo = object
    x: int
  proc `=destroy`*(a: var Foo) {.inline.} =
    echo ("in destroy", a.x)
  proc main =
    var v: seq[Foo]
    v.add Foo(x: 10)
    v.add Foo(x: 11)
    v.add Foo(x: 12)
    echo "ok0"
    echo v
    echo "ok1"
    echo v
    echo "ok2"
  main()

with --gc:arc and --gc:refc, this gives:

ok0
@[(x: 10), (x: 11), (x: 12)]
ok1
@[(x: 10), (x: 11), (x: 12)]
ok2
("in destroy", 10)
("in destroy", 11)
("in destroy", 12)

as it should

timotheecour avatar Jun 28 '21 03:06 timotheecour

Thanks for forwarding the issue.

When I run the same code with Nim v1.4.8 and any of --gc=arc|orc|refc, I have a slightly different output.

[]
ok0
("in destroy", 12)
[(x: 10), (x: 11), (x: 12)]
ok1
("in destroy", 12)
[(x: 10), (x: 11), (x: 12)]
ok2

Which is still wrong. I'll see if I can look into it.

Clonkk avatar Jun 28 '21 09:06 Clonkk

Updated this issue with Nim 2.0 :

import cppstl/std_vector

type Foo = object
  x: int

proc `=destroy`*(a: Foo) =
  echo ("=destroy", a.x)

proc main =
  var v = initCppVector[Foo]()

  echo v
  v.add Foo(x: 10)
  v.add Foo(x: 11)
  echo v
  v.add Foo(x: 12)
  v.add Foo(x: 13)

  echo "-----------"
  echo v
  # Clear should call object destructor
  v.clear()

main()

Output :

[]
("=destroy", 11)
[(x: 10), (x: 11)]
-----------
("=destroy", 13)
[(x: 10), (x: 11), (x: 12), (x: 13)]
("=destroy", 13)
("=destroy", 12)
("=destroy", 11)
("=destroy", 10)

Somehow the act of printing the std::vector invoke a destructor on that added element. No idea why.

Any insight @jmgomez ?

Clonkk avatar Jun 03 '24 21:06 Clonkk

Updated this issue with Nim 2.0 :

import cppstl/std_vector

type Foo = object
  x: int

proc `=destroy`*(a: Foo) =
  echo ("=destroy", a.x)

proc main =
  var v = initCppVector[Foo]()

  echo v
  v.add Foo(x: 10)
  v.add Foo(x: 11)
  echo v
  v.add Foo(x: 12)
  v.add Foo(x: 13)

  echo "-----------"
  echo v
  # Clear should call object destructor
  v.clear()

main()

Output :

[]
("=destroy", 11)
[(x: 10), (x: 11)]
-----------
("=destroy", 13)
[(x: 10), (x: 11), (x: 12), (x: 13)]
("=destroy", 13)
("=destroy", 12)
("=destroy", 11)
("=destroy", 10)

Somehow the act of printing the std::vector invoke a destructor on that added element. No idea why.

Any insight @jmgomez ?

@Clonkk sorry I missed this. Likely is creating a temp value when doing echo which triggers a copy. Will test it out when I have some time

jmgomez avatar Aug 18 '24 14:08 jmgomez

Took a look, the issue is in the $ implementation, by implementing it like below instead it behaves as expected:

proc `$`*[T](v: CppVector[T]): string =
  ## The `$` operator for CppVector type variables.
  ## This is used internally when calling `echo` on a CppVector type variable.
  runnableExamples:
    var
      v = initCppVector[int]()
    doAssert $v == "[]"

    v.add(100)
    v.add(200)
    doAssert $v == "[100, 200]"
  #
  if v.empty():
    result = "[]"
  else:
    result = "["
    for idx in 0.csize_t ..< v.size():
      result.add($v[idx])
      if idx < v.size() - 1:
        result.add(", ")
    result.add("]")
    ```

jmgomez avatar Aug 18 '24 21:08 jmgomez

I'll make a PR to fix that. Thanks for taking a look

Clonkk avatar Aug 18 '24 22:08 Clonkk