deprecates newSeqUninitialized replaced by unsafeseqs.newSeqUninit

[ringabout]

ref #19727

[juancarlospaco]

Should not be too complicated to make newSeqUninit work in JS targets ?, just return an Array of null. 🤔

[arnetheduck]

missing type constraint for T?

[ringabout]

Nope, it's intended. There is an example in the runnableExample to demonstrate how to use it with types having destructors (credits to @Araq).

[arnetheduck]

"be cautious" is kind of .. unsettling though - ie it feels like a sentence saying that it randomly might or might not work depending on the whims of the compiler, the complexity of the object graph or the mood of the developer

[arnetheduck]

also, nodestroy on the whole proc like that would likely lead to other bugs - ie if you (accidentally) create some temporaries during construction - sounds like a good way to mess up the compiler - not sure it's good to recommend that kind of footgun

[Araq]

It's a sharp tool but it fits newSeqUninit which is a very sharp tool too. However, we can move these to a new module unsafeseqs and then distinguish between newSeqUninit which has the constraint supportsCopyMem(T) and newSeqUnsafe which lacks the constraint.

[arnetheduck]

well, that's reasonable along with a better name - this version is not sharp at all beyond its name for plain types ("uninitialised" vs "broken GC with lots of gotchas")

for the gc case, one would almost need to have a template newSeqInitIt(size, body): {.nodestroy.}: let it = ...; body to make it .. reasonable. nodestroy is a bit of a shotgun.

[ringabout]

we can move these to a new module unsafeseqs and then distinguish between newSeqUninit which has the constraint supportsCopyMem(T) and newSeqUnsafe which lacks the constraint.

done

[AmjadHD]

newSeqUninit is safe why is it in unsafeseqs ?

[ringabout]

Probably"unsafeseqs" is not a descriptive name for both of functions. I added both of them there instead of system lest they would break important packages. We can also move newStringUninit to std/strbasics and call this "seqbasics" or something. Or a new module called initutils for all of these initializations.

[ringabout]

There are two packages(arraymancer, neo) are using newSeqUninit? Should we fix these packages or probably move it to sequtils if that helps?

[arnetheduck]

well, that's reasonable along with a better name - this version is not sharp at all beyond its name for plain types ("uninitialised" vs "broken GC with lots of gotchas")

for the gc case, one would almost need to have a template newSeqInitIt(size, body): {.nodestroy.}: let it = ...; body to make it .. reasonable. nodestroy is a bit of a shotgun.

[arnetheduck]

uh, oops, wrong button - the approve was meant to be a comment saying that it would be nice with more protection against silly cases.

in particular, it's very hard to know if some object has ref members, but many objects are plain enough that it should work.

[arnetheduck]

for threads and many other use cases, it would actually be nice to have a way of saying [T: supportsCopyMem(T) in the generic constraints

[mratsim]

for threads and many other use cases, it would actually be nice to have a way of saying [T: supportsCopyMem(T) in the generic constraints

Can supportsCopyMem be a concept constraint?

[mratsim]

There are two packages(arraymancer, neo) are using newSeqUninit? Should we fix these packages or probably move it to sequtils if that helps?

Arraymancer's newSeqUninit is a custom one because newSeqUninitialized didn't support all the types needed (no complex or enums iirc), is the issue an ambiguous symbol? In any case it can be superceded by this one.

[metagn]

is the issue an ambiguous symbol?

Yep, but also with neo as well as arraymancer:

neo/dense.nim(1144, 31) Error: ambiguous call; both dense.newSeqUninit(len: Natural) [func declared in neo/dense.nim(1089, 6)] and system.newSeqUninit(len: Natural) [proc declared in Nim/lib/system.nim(1614, 8)] match for: (int32)

arraymancer/linear_algebra/helpers/least_squares_lapack.nim(80, 32) Error: ambiguous call; both system.newSeqUninit(len: Natural) [proc declared inNim/lib/system.nim(1614, 8)] and sequninit.newSeqUninit(len: Natural) [func declared in arraymancer/private/sequninit.nim(15, 6)] match for: (int)

arraymancer/tensor/exporting.nim(40, 29) Error: ambiguous call; both system.newSeqUninit(len: Natural) [proc declared in Nim/lib/system.nim(1614, 8)] and sequninit.newSeqUninit(len: Natural) [func declared in arraymancer/private/sequninit.nim(15, 6)] match for: (Natural)

Libraries could define it with a when not declared(newSeqUninit): guard.

[mratsim]

Arraymancer will be compatible: mratsim/Arraymancer#589

[beef331]

Can supportsCopyMem be a concept constraint?

Yes, but as I understand it there should be a containsRef typetrait, which would be a better constraint checking if a type has a seq, string, ref or ptr anywhere inside itself.

[Varriount]

Why put this in a module? It's a single function.

[arnetheduck]

Yes, but as I understand it there should be a containsRef typetrait

this is often not enough - ie anything that relies on magic that the compiler generates behind your back (memory allocations like closures, destrutors etc) are usually a no-no for building core infrastructure supporting threads and other advanced use cases - for this, precise primitives are needed in the language along with guarantees about how they will evolve in the future so that one can describe functions that are safe for what in C would have been termed POD - C++ is a good journey to study where pod was later refined to https://en.cppreference.com/w/cpp/language/classes#Trivial_class in order to be able to generate optimal code for each kind of layout.

supporsCopyMem is kind of a sledgehammer that you can start with to get rid of all of the above magic (though it is not sufficient for memcmp) - if we go down the containsRef route, a more complete review would be in order to ensure full expressivity for other case - doable, but I think a more involved piece of work.

[arnetheduck]

this is not a primary use case - I think the documentation should generally recommend newSeqUninit and only fall back to newSeqUnsafe for cases that involve complex types - I also think it's prudent to clearly establish the meaning of "unsafe" in this context, otherwise it's just a vacuous word for unknown bounds which makes it difficult to reason about in usage. ie "how is this unsafe?" for example, it is not unsafe in the same way addr is unsafe.

[Araq]

supportsCopyMem is kind of a sledgehammer that you can start with

Actually, it's not supposed to be a sledgehammer, it is the beginning of a classification system with crystal clear naming and C++ could learn something here too. ;-)

But this all feels rather misguided when we could optimize sequtils.repeat and promote its usage.

[arnetheduck]

But this all feels rather misguided when we could optimize sequtils.repeat and promote its usage.

repeat doesn't apply in most use cases that we have for it: one use case is that you have some data in one place (a disk, some other in-memory structure etc) and want to put it into a seq in particular. The need occurs in two ways depending on context: both when creating a new seq and when growing an existing one (ie setLenUninit) - low-level utilities of this kind are needed to support libraries that can expose a better interface to users while still being efficient enough to not be embarrassingly slow (ie we could use raw memory buffers and not use seq at all to get back to "normal" performance expected from a compiled language, but that's not a desire:able outcome - until this API exists, seq is crippled).

In measuring performance, the zeroing is often dominant even for disk reads when the data is in cache, but it truly stands out in dumb low-level memory copies from a to b.

[arnetheduck]

crystal clear naming

I'd tend to prefer names that explain the primitives rather than complex combinations thereof - ie supportsCopyMem is a combination of "no refs" and "no destructors" - its name alludes to the fact that a single function is safe to use ("copyMem") rather than a broad set of operations that depend on said primitves. I always feel uncomfortable using it for any other purposes than copyMem for this reason, because who knows what it actually means beyond supporting that single operation.

[Varriount]

This should not be in its own module. If keeping it out of system is absolutely necessary, put it in a module named something like unsafe or something.

[ringabout]

Succeeded by #22739