Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecates newSeqUninitialized replaced by unsafeseqs.newSeqUninit #22586

Closed
wants to merge 8 commits into from
Closed

deprecates newSeqUninitialized replaced by unsafeseqs.newSeqUninit #22586

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fd076d6
deprecates `newSeqUninitialized` replaced by `newSeqUninit`
ringabout Aug 29, 2023
b39f174
adds a changelog
ringabout Aug 29, 2023
73a5418
Update changelog.md
ringabout Aug 29, 2023
eaf417e
Update lib/system.nim
ringabout Aug 29, 2023
a448de2
adapt to reviews
ringabout Aug 30, 2023
240a085
adds one more link
ringabout Aug 30, 2023
1886ca1
Update changelog.md
ringabout Aug 30, 2023
187f383
Update changelog.md
ringabout Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

[//]: # "Deprecations:"

- Deprecates `system.newSeqUninitialized`, which is replaced by `newSeqUninit`.

[//]: # "Removals:"

Expand Down
40 changes: 32 additions & 8 deletions lib/system.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ proc newSeq*[T](len = 0.Natural): seq[T] =
##
## See also:
## * `newSeqOfCap <#newSeqOfCap,Natural>`_
## * `newSeqUninitialized <#newSeqUninitialized,Natural>`_
## * `newSeqUninit <#newSeqUninit,Natural>`_
newSeq(result, len)

proc newSeqOfCap*[T](cap: Natural): seq[T] {.
Expand Down Expand Up @@ -1611,7 +1611,36 @@ when notJSnotNims and defined(nimSeqsV2):
include "system/seqs_v2"

when not defined(js):
proc newSeqUninitialized*[T: SomeNumber](len: Natural): seq[T] =
proc newSeqUninit*[T](len: Natural): seq[T] =
## Creates a new sequence of type `seq[T]` with length `len`.
##
## Note that the sequence will be
## uninitialized. After the creation of the sequence you should assign
## entries to the sequence instead of adding them.
runnableExamples:
# newSeqUninit can be safely used for types, which don't contain
# managed memory or have destructors
var x = newSeqUninit[int](3)
assert len(x) == 3
x[0] = 10

# be cautious to use it with types with managed memory or destructors
ringabout marked this conversation as resolved.
Show resolved Hide resolved
proc initStringSeq(x: Natural): seq[string] {.nodestroy.} =
result = newSeqUninit[string](x)
for i in 0..<x: result[i] = "abc"

let s = initStringSeq(10)
assert len(s) == 10
assert s[0] == "abc"

result = newSeqOfCap[T](len)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing type constraint for T?

Copy link
Member Author

@ringabout ringabout Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newSeqUninit is safe why is it in unsafeseqs ?

Copy link
Member Author

@ringabout ringabout Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

when defined(nimSeqsV2):
cast[ptr int](addr result)[] = len
else:
var s = cast[PGenericSeq](result)
s.len = len

proc newSeqUninitialized*[T: SomeNumber](len: Natural): seq[T] {.deprecated: "Use newSeqUninit instead", inline.} =
## Creates a new sequence of type `seq[T]` with length `len`.
##
## Only available for numbers types. Note that the sequence will be
Expand All @@ -1623,12 +1652,7 @@ when not defined(js):
## assert len(x) == 3
## x[0] = 10
## ```
result = newSeqOfCap[T](len)
when defined(nimSeqsV2):
cast[ptr int](addr result)[] = len
else:
var s = cast[PGenericSeq](result)
s.len = len
result = newSeqUninit[T](len)

proc newStringUninit*(len: Natural): string =
## Returns a new string of length `len` but with uninitialized
Expand Down