Nim icon indicating copy to clipboard operation
Nim copied to clipboard

remove `shallow` usages for ORC

Open ringabout opened this issue 1 year ago • 3 comments

shallow doesn't work for ARC/ORC.

proc shallow*[T](s: var seq[T]) {.noSideEffect, inline.} =
  ## Marks a sequence `s` as `shallow`:idx:. Subsequent assignments will not
  ## perform deep copies of `s`.
  ##
  ## This is only useful for optimization purposes.
  if s.len == 0: return
  when not defined(js) and not defined(nimscript) and not defined(nimSeqsV2):
    var s = cast[PGenericSeq](s)
    s.reserved = s.reserved or seqShallowFlag

Should we remove it from system for ARC/ORC? or deprecated it since it can only be used for optimizations.

ringabout avatar Jul 29 '22 12:07 ringabout

or deprecated it since it can only be used for optimizations.

it's not only optimizations - it significantly changes semantics because the shallow operation enables shared mutable instances (which semantically is problematic in general) which in turn changes the meaning of programs using it, beyond optimisation (it's observable) - removing completely for orc seems reasonable.

arnetheduck avatar Aug 09 '22 08:08 arnetheduck

it's not only optimizations - it significantly changes semantics because the shallow operation enables shared mutable instances (which semantically is problematic in general) which in turn changes the meaning of programs using it, beyond optimisation (it's observable)

True, but it is documented at least => "This is only useful for optimization purposes."

removing completely for orc seems reasonable.

I will go for it, though I cannot manage to fix every important package by myself very soon. shallowCopy has broken lots of packages in my PR(defaults to ORC).

ringabout avatar Aug 09 '22 08:08 ringabout

True, but it is documented at least

documentation is wrong - perhaps whoever wrote it didn't consider the semantic consequences, but they are there nonetheless, and likely relied upon in edge-case code which makes it harder to detect - better remove and make it break visibly.

shallowCopy has broken lots of packages in my PR(defaults to ORC).

this is the easy part of moving to orc ;) at least it breaks visibly.

arnetheduck avatar Aug 09 '22 17:08 arnetheduck

Thanks for your hard work on this PR! The lines below are statistics of the Nim compiler built from 9753dd0a1bd4a9993941dcd1e39d930bd2bb8bf0

Hint: mm: orc; threads: on; opt: speed; options: -d:release 163644 lines; 12.003s; 841.363MiB peakmem

github-actions[bot] avatar Aug 23 '22 16:08 github-actions[bot]