Feat: stdlib: adds system.string.setLenUninit

[ZoomRmc]

Adds system.setLenUninit for the string type. Allows setting length without initializing new memory on growth.

• Required for a follow-up to add overload add(a: var string, b: openArray[char]) #15951
• Accompanies ref #19727; implement setLenUninit for seqsv2 #22767 (ref newSeqUninitialized over-constrained, setLenUninitialized missing #19727) but for strings

• sysstr housekeeping:
  • Removed redundant len and reserved sets already performed by prior rawNewStringNoInitcalls.

[ZoomRmc]

systr.setLengthStrUninit fails with mm:refc on Linux.
Seems my naïve approach is wrong.
Can this be made to work without introducing new compiler procs?

[arnetheduck]

the same for seq would be nice, ie strings are pretty niche at the end of the day.
never mind, forgot that it was already fixed :)

[arnetheduck]

ping, would be nice to see this PR merged

[ZoomRmc]

I think I was stuck on not being sure if it's done and both string versions actually work, since some of CI errors at the time were unrelated.

Those string/seq impls is a mess of conditionals and I don't trust locally-run tests.

[ZoomRmc]

Nim/lib/system.nim

Lines 2316 to 2326 in 2e45f61

	when notJSnotNims:
	when defined(nimSeqsV2):
	{.noSideEffect.}:
	let str = unsafeAddr s
	setLengthStrV2Uninit(cast[ptr NimStringV2](str)[], newlen)
	else:
	{.noSideEffect.}:
	when hasAlloc:
	setLengthStrUninit(s, newlen)
	else:
	s.setLen(n)

Adding a hasAlloc check and and else-downgrade to setLen for the old string branch was necessary for this to compile. I'm not sure if it's correct as I'm not sure what constitutes the string type at this point.

[Araq]

This is not what the original code did and a distraction. Bring back the old version.

[ZoomRmc]

Again, if we don't return early from this case (and checking for nil is cleraly an "early return" kind of branch),
we're setting len and the nul-byte for the result again at lines 229-230

Nim/lib/system/sysstr.nim

Lines 229 to 230 in 2e45f61

	result.len = n
	result.data[n] = '\0'

.

mnewString sets the len and the null-byte on each call for the returned string (though, the null-byte implicitly, by zeroMem call):

Nim/lib/system/sysstr.nim

Lines 62 to 65 in 2e45f61

	proc mnewString(len: int): NimString {.compilerproc.} =
	result = rawNewStringNoInit(len)
	result.len = len
	zeroMem(addr result.data[0], len + 1)

Original code for the s == nil and n != 0 went like this:

# start of the mnewString call
result = rawNewStringNoInit(n)
result.len = n
zeroMem(addr result.data[0], n + 1) # n (or len in the proc) + 1 sets the null-byte
# after-coditional tail of setLengthStr
result.len = n
result.data[n] = '\0'

So this is redundant.

How about I separate the nil case into separate if statement to signify it's an early-return case?

[ZoomRmc]

I mean, it could look like this:

proc setLengthStr(s: NimString, newLen: int): NimString {.compilerRtl.} =
  let n = max(newLen, 0)
  if s == nil: # early return check
    if n == 0:
      return s
    else:
      return mnewString(n) # sets everything required
  if n <= s.space:
    result = s
  else:
    let sp = max(resize(s.space), n)
    result = rawNewStringNoInit(sp) # len and null-byte not set
    copyMem(addr result.data[0], unsafeAddr(s.data[0]), s.len)
    zeroMem(addr result.data[s.len], n - s.len)
  result.len = n
  result.data[n] = '\0'

This is more explicit and in terms of logic completely equal to the original version, barring reduntant field touches, but I'm not totally sure what's "distracting" to you.

[ZoomRmc]

I think my previous version is more succinct, but indeed it might be better to change the elif at ln.227 to a separate if to separate the early-return statement.

[Araq]

Apart from my code review remarks, seems fine.

[ZoomRmc]

So I went with expanding that early return conditional at line 232 a bit and added some internal docs. If it's still not up your alley I can change the code to whatever.

If it's ok as is, do you want me to squash it?

BTW, you can't currently build docs with --docInternal for system due to assertion defect in semtypes:

...assertions.nim(34)       raiseAssert
Error: unhandled exception: semtypes.nim(2471, 7) `c.graph.sysTypes[tySequence] == nil`  [AssertionDefect]```