Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
add std/smartptrs #17333
add std/smartptrs #17333
Changes from all commits
3273fd7
b4fa432
421cf08
ea5fe84
7b329bc
4aae28e
b6af261
f9bf78d
5e3eb8d
57e18ad
162db95
120716f
5566515
6f30a8c
7992e60
ef0819e
fe8b805
546cfb9
d59eca6
d62f0e2
5bbbdc5
8503a75
2910418
290097c
9321cc6
246bff5
df0b217
275887c
abd5937
7a85e22
6b7226a
fd9bdf5
56afd30
9c9f915
3111c8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mention in #18450 (comment) for the other PR, this is what's needed:
which also ends up producing correct results for the C++ example i gave, unlike what the other APIs do.
read the whole thread starting here for more context: #18450 (comment)
I also wonder whether we actually need
proc newUniquePtr[T](val: sink Isolated[T]): UniquePtr[T] {.nodestroy.} =
given thatproc newUniquePtr*[T](U: typedesc[T]): UniquePtr[T] =
seems simpler, more correct, and more performant (no copy)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. Don't you want the unique pointer to point to some valid object? Your proc doesn't do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm allocating on the heap only once instead of allocating in caller scope (eg on stack) and then allocating on heap and copying; it's more efficient and analogous to C++ placement new on which C++
make_unique
relies, avoids complications of isolated, and is also more correct wrt destructors being called as you can see in #18450 (comment)Caller can then, if needed, assign values in the heap allocated object, eg:
prints:
It's also the correct way to wrap C++ objects (eg with non-trivial constructor): it allows the C++ object to be constructed exactly once, and then destroyed when the unique_ptr goes out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timotheecour its already taken care of https://github.com/nim-lang/threading/blob/master/threading/smartptrs.nim#L48 Its not more efficient though, you should use the sink one (unless you have to) as I said in #17333 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is not as efficient as direct inplace construction. Not because of the lack of moves, but because the compiler doesn't know that in
p[]
there is nothing to destroy yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's measuring artifacts of the current implementation, in particular NVRO (which is unfortunately currently fragile) or the lack thereof. In theory it's a pessimization as you mitigated the compiler's ability to reason about the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand the reasoning here; in current code, you:
1 construct an object Foo in the stack
2 allocate sizeof(Foo) on the heap
3 copy Foo
with proposed code you:
1 allocate sizeof(Foo) on the heap
2 construct in-place
in all possible scenarios, it should be at least as fast, and usually faster; eg it allows placement-new with C++, or simply just constructing in-place the fields you care about updating from their default value (if some of the memory is to remain 0-initialized, you don't pay for it); you also don't have extra ctor/dtor to worry about, in particular for C++ objects.
n element version
Finally, this generalizes in a straightforward way to C++'s array version of make_unique, which we should add at some point; it allows dynamic allocation of n objects without having to use a seq (eg so you can interface with C++ or avoid relying on gc for allocating n elements); there are 2 approaches to consider:
UniquePtrN
type with an extra fieldn: int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just pointing it out. I had totally missed it before.
Why not use a seq instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your're correct, however, the code is still not as efficient as it could be if we had proper "in-place" construction. I think we could do proper in-place construction for the construct
toSinkParam ObjConstr(x: x, y: y, ...)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for c++ objects, it should already be possible today without a language extension (via importcpp of
new(buf) T
), although i haven't tried yetfor nim objects, it shouldn't be hard to extend
semObjConstr
to take an optional input buffer address, eg:in any case, these can be added later, and
proc newUniquePtrN*[T](U: typedesc[T]): UniquePtrN[T] =
can be introduced before those exist; code would just get auto-optimized oncep[] = Foo(a1: 1, a2: 2)
starts working as in-place construction