cello icon indicating copy to clipboard operation
cello copied to clipboard

replace shallowCopy for ARC/ORC

Open ringabout opened this issue 1 year ago • 6 comments

Hello, shallowCopy has been removed for ARC/ORC since it does a deep copy with ARC/ORC → https://github.com/nim-lang/Nim/pull/20070

I don't know a good alternative for shallowCopy.

ref https://github.com/nim-lang/Nim/pull/19972

ringabout avatar Aug 31 '22 06:08 ringabout

Hi @ringabout, thank you for the PR. But I must say I am confused - isn't there a mechanism to share storage with ORC? The whole point of the rotate function and RotatedString is to return something that behaves like a rotated string, but actually does not copy the whole memory.

andreaferretti avatar Aug 31 '22 07:08 andreaferretti

isn't there a mechanism to share storage with ORC?

Imo, strings and seqs cannot be shared with ARC/ORC in a safe way. It can be either moved or copied. Sometimes it can be optimized using sink, which let the compiler choose whether to copy or move.

ringabout avatar Aug 31 '22 08:08 ringabout

I think the only workaround that can actually work in this case without changing semantics is creating some ref object type that stores the string internally (even though this will lead to double indirection which kinda sucks).

Yardanico avatar Sep 06 '22 14:09 Yardanico

You could use a cow string implementation internally.

planetis-m avatar Sep 08 '22 14:09 planetis-m

I think I will go with double indirection as soon as I have time

andreaferretti avatar Sep 08 '22 15:09 andreaferretti

I think the only workaround that can actually work in this case without changing semantics is creating some ref object type that stores the string internally (even though this will lead to double indirection which kinda sucks).

I'm not sure how to achieve that while keeping this test work

proc rotate*(s: string, i: int): RotatedString
test "underlying strings are shared":
  var
    x = "Hello, world"
    y = x.rotate(5)

  y[0] = 'f'
  check x[5] == 'f'

ringabout avatar Sep 19 '22 14:09 ringabout