rename newSeqUninit in order to avoid conflicts with upstream

[ringabout]

ref nim-lang/Nim#22739

It's a private function so it shouldn't affect backwards compatibility.

[Vindaar]

I'm confused as to why we cannot keep the when not declared logic? I haven't fully understood the stdlib changes yet, sorry.

[ringabout]

It seems that

Arraymancer/src/arraymancer/laser/tensor/datatypes.nim

Line 37 in 875ee6b

storage*: CpuStorage[T] # 8 bytes

uses refs, which is not supported by newSeqUninit.

But newSeqUninit is unavailable to types, which contain managed memory or have destructors.

[Vindaar]

Ohh, now I see. My confusion was that I assumed this PR here nim-lang/Nim#22586 was merged already and based on Mamy's PR #591 I further assumed it was a drop in replacement for our custom one.

And yes, the CpuStorage can either hold regular POD types or references. Thanks for the explanation.

What is the reason though for the restriction of the stdlib version to types that can be memcopied? The code currently doesn't do anything that seems to rely on that, no?

I don't have a strong opinion on the name, newSeqUninit2 is fine by me. Just trying to understand the restrictions.

[ringabout]

I think type bounding operations requires types to be initialized preemptively.

For instance, destructors or other type bounding operations might not work properly since Ref might be anything.

var x = newSeqUninit[Ref](10)

[Vindaar]

But then what does our current code do? Or in other words, if I currently write:

type
  Foo = ref object
    x: float

var x = newSeqOfCap[Foo](10)
x.setLen(10)

wouldn't that be undefined behavior if I understand you correctly?

[ringabout]

setLen will initialize the sequence in v2 since its original length is smaller than the new length

var x = newSeqOfCap[Foo](10)
x.setLen(10)

so it is just another way of writing vat x = newSeq[Foo](10) with the same efficiency.

proc setLen[T](s: var seq[T], newlen: Natural) {.nodestroy.} =
  {.noSideEffect.}:
    if newlen < s.len:
      shrink(s, newlen)
    else:
      let oldLen = s.len
      if newlen <= oldLen: return
      var xu = cast[ptr NimSeqV2[T]](addr s)
      if xu.p == nil or (xu.p.cap and not strlitFlag) < newlen:
        xu.p = cast[typeof(xu.p)](prepareSeqAddUninit(oldLen, xu.p, newlen - oldLen, sizeof(T), alignof(T)))
      xu.len = newlen
      for i in oldLen..<newlen:
        xu.p.data[i] = default(T)

[Vindaar]

Ah, I see. But then that implies our implementation, newSeqUninit2 now, is "broken" and doesn't actually do an "uninit" implementation anymore.

So we need to merge the logic for newSeqUninit2 to be something like this:

func newSeqUninit2*[T](len: Natural): seq[T] {.inline.} =
  ## Creates an uninitialzed seq if the type is plain old data. Falls back
  ## to a normal `newSeq` call for any non trivial data type.
  when supportsCopyMem(T):
    result = newSeqUninit[T](len)
  else:
    result = newSeq[T](len)

so that at least we get the desired behavior for mem copyable types.

[ringabout]

Indeed, I agree. It also needs to be combined with when declared(newSeqUninit).

[Vindaar]

Cool! Will you make the change or should I do it?

[ringabout]

Feel free to take over this PR, or do it in the following PRs. Thank you in advance!

[Vindaar]

Sorry for the delay. Will take care of this soon. Yesterday was a bit stressful. :)

[mratsim]

I think we can just delete newSeqUninit in Arraymancer actually, with the Laser PR, the storage for POD datatypes changed to ptr UncheckedArray:

https://github.com/mratsim/Arraymancer/blob/875ee6bf595a220a08827adeaee8a89722d2c8f2/src/arraymancer/laser/tensor/datatypes.nim#L39C1-L47

so CPU storage is only used for ref types and strings but the current impl will likely cause issues with destructors and we aren't optimizing for strings/ref types.

In fact, CpuStorage uses newSeq for alloc of tensors

Arraymancer/src/arraymancer/laser/tensor/datatypes.nim

Lines 86 to 101 in 875ee6b

	proc allocCpuStorage*[T](storage: var CpuStorage[T], size: int) =
	## Allocate aligned memory to hold `size` elements of type T.
	## If T does not supports copyMem, it is also zero-initialized.
	## I.e. Tensors of seq, strings, ref types or types with non-trivial destructors
	## are always zero-initialized. This prevents potential GC issues.
	when T is KnownSupportsCopyMem:
	when not defined(gcDestructors):
	new(storage, finalizer[T])
	else:
	new(storage)
	storage.memalloc = allocShared(sizeof(T) * size + LASER_MEM_ALIGN - 1)
	storage.isMemOwner = true
	storage.raw_buffer = align_raw_data(T, storage.memalloc)
	else: # Always 0-initialize Tensors of seq, strings, ref types and types with non-trivial destructors
	new(storage)
	storage.raw_buffer.newSeq(size)

and newSeqUninit only for intermediate sequences when interfaces with C libraries like Lapack for which the stdlib would be a drop-in replacement.

https://github.com/search?q=repo%3Amratsim%2FArraymancer%20newSequninit&type=code

[Vindaar]

Good point actually.
The one exception is the toRawSeq implementation, but given that this is deprecated for good reason, I'd simply use a regular newSeq there instead and go ahead and removing our implementation.