Improve std.typecons.Unique #3139

Merged
merged 16 commits into from Apr 24, 2015

Conversation

Projects
None yet
7 participants
@mrkline
Contributor

mrkline commented Apr 2, 2015

Whenever D is brought up to the general programming public, the garbage collector is quickly raised as a point of contention. Regardless of how legitimate or well-informed these concerns are or are not, it would be a massive public relations boon — and great for the language, to boot — if we could trot out a solid set of RAII-based smart pointers for those who prefer to use them. We have a solid start in std.typecons.Unique and std.typecons.RefCounted. Unfortunately, these seem to be victims of bit rot and compiler bugs of days long gone.

This is the first of several pull requests that attempt to clean these up. An overview of the changes in this request is as follows (updated 2015-04-21):

  • Unique uses malloc and free instead of the GC for backing memory storage. Unfortunately this rules out nested types for the time being (as emplace cannot set the frame pointer for closures).
  • std.algorithm.move is used instead of a special release member function. Whether by design or by happy accident, move transfers ownership between Unique pointers in a very similar manner to C++'s std::move with std::unique_ptr. Along with being a familiar paradigm to C++ users, using move to transfer ownership makes more intuitive sense and builds consistency with the rest of Phobos.
  • With std.algorithm.move transferring ownership, release is deprecated.
  • Unique.create has transformed into a freestanding unique function. Regardless of whether or not there is language support for checking uniqueness, a utility function that creates a Unique, taking the same arguments as the underlying type's constructor, is extremely useful, as demonstrated by the addition of make_unique to C++14.
  • Constructors taking a pointer have been removed, since we now control allocation ourselves.
  • A new method, get, returns the underlying pointer, for use in functions and code that do not play a role in the life cycle of the object. Smart pointers are as much about ownership semantics as they are about allocating and freeing memory, and non-owning code should continue to refer to data using a raw pointer or a reference.

This pull request is not meant to be merged prima facie, but to initiate a dialogue. I strongly believe this is a good step in the right direction and would love to hear commentary from the core devs. Perhaps some of the implementation details need to be hashed out some more, but we can all agree that a stronger showing from the smart pointers in std.typecons will only bolster D and Phobos.

Improve std.typecons.Unique
Whenever D is brought up to the general programming public,
the garbage collector is quickly raised as a point of contention.
Regardless of how legitimate or well-informed these concerns are,
it would be a massive public relations boon --- and great for the language,
to boot --- if we could trot out a solid said of RAII-based smart pointers
for those who prefer to use them. We have a solid start in
std.typecons.Unique and std.typecons.RefCounted.
Unfortunately, these classes seem to be victims of bit rot and
compiler bugs of days long gone.

An overview of the changes in this commit is as follows:

- Unique's underlying data now uses malloc and free
  instead of the garbage collector. Given that many people use RAII
  smart pointers to escape the GC, it seems to make more sense to
  avoid it here. On a related note, isn't delete deprecated?
  The current destructor uses it.

- std.algorithm.move is used instead of a special release
  member function. Whether by design or by happy accident,
  move transfers ownership between Unique pointers in a very
  similar manner to C++'s std::move with std::unique_ptr.
  Along with being a familiar paradigm to C++ users,
  using move to transfer ownership makes more intuitive sense
  and builds consistency with the rest of Phobos.

- With std.algorithm.move transferring ownership, release now just
  frees the underlying pointer and nulls the Unique.

- Unique.create is no longer compiled out using version(None).
  Regardless of whether or not there is language support for
  checking uniqueness, a utility function that creates a Unique,
  taking the same arguments as the underlying type's constructor,
  is extremely useful, as demonstrated by the addition of
  make_unique to C++14.

- Because Unique.create is now in place and Unique is backed with
  malloc, constructors taking a pointer have been removed.
  This encourages the use of create as the idiomatic,
  consistent method to, well, create Unique objects.
  If one can only get a Unique by calling create or moving another
  into it, we also ensures uniqueness in one fell swoop.

- A new method, get, returns the underlying pointer, for use in
  functions and code that do not play a role in the life cycle
  of the object. Smart pointers are as much about ownership
  semantics as they are about allocating and freeing memory,
  and non-owning code should continue to refer to data using a raw
  pointer or a reference.
@mrkline

View changes

std/typecons.d
+ immutable size_t allocSize = T.sizeof;
+
+ void* rawMemory = enforce(malloc(allocSize), "malloc returned null");
+ u._p = cast(RefT)rawMemory;

This comment has been minimized.

@mrkline

mrkline Apr 2, 2015

Contributor

I appear to be having a segfault on classes that have a reference to their context, such as the one on the third unit test, on line 281. When I step through create in GDB, rawMemory is correctly set to our newly allocated memory, but after stepping past this line, u._p is some bad address, such as 0x14. It's like the assignment is ignored or the cast messes with the address somehow. Is this some failing of my understanding? The D site says that "Casting a pointer type to and from a class type is done as a type paint (i.e. a reinterpret cast)." so I'm not sure how this is happening.

@mrkline

mrkline Apr 2, 2015

Contributor

I appear to be having a segfault on classes that have a reference to their context, such as the one on the third unit test, on line 281. When I step through create in GDB, rawMemory is correctly set to our newly allocated memory, but after stepping past this line, u._p is some bad address, such as 0x14. It's like the assignment is ignored or the cast messes with the address somehow. Is this some failing of my understanding? The D site says that "Casting a pointer type to and from a class type is done as a type paint (i.e. a reinterpret cast)." so I'm not sure how this is happening.

This comment has been minimized.

@mrkline

mrkline Apr 2, 2015

Contributor
Breakpoint 1, std.typecons.Unique!(std.typecons.__unittestL276_3().C).Unique.create!().create() (
    __HID19=0x7fffffffe0d8) at std/typecons.d:101
101         u._p = cast(RefT)rawMemory;
(gdb) print rawMemory
$1 = (void *) 0x92d860
(gdb) n
111             void[] init = typeid(T).init[];
(gdb) print u._p
$2 = (struct std.typecons.__unittestL276_3.C *) 0x7fffffffe060

ಠ_ಠ

@mrkline

mrkline Apr 2, 2015

Contributor
Breakpoint 1, std.typecons.Unique!(std.typecons.__unittestL276_3().C).Unique.create!().create() (
    __HID19=0x7fffffffe0d8) at std/typecons.d:101
101         u._p = cast(RefT)rawMemory;
(gdb) print rawMemory
$1 = (void *) 0x92d860
(gdb) n
111             void[] init = typeid(T).init[];
(gdb) print u._p
$2 = (struct std.typecons.__unittestL276_3.C *) 0x7fffffffe060

ಠ_ಠ

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 2, 2015

Great to see some effort here. One quick thing that caught my eye is Unique's underlying data now uses malloc and free instead of the garbage collector which is sub-optimal, because it is perfectly legal to use Unique with GC managed data. Ideally it should be possible to define initializer and finalizer for Unique!T different for each T, so that any custom allocator could be used. I am not sure if this belongs to scope of this PR though.

Great to see some effort here. One quick thing that caught my eye is Unique's underlying data now uses malloc and free instead of the garbage collector which is sub-optimal, because it is perfectly legal to use Unique with GC managed data. Ideally it should be possible to define initializer and finalizer for Unique!T different for each T, so that any custom allocator could be used. I am not sure if this belongs to scope of this PR though.

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 2, 2015

Contributor

My rationale behind doing so was that if we remove constructors accepting RefT and promote create as the way to make a Unique, we can completely control the life cycle of the underlying object T from start to finish. This is the entire point of Unique, is it not? And if we completely control the life cycle, then there is no reason to rely on the GC. If you think that's too ambitious to start with or don't agree with the idea, I can certainly shelve it for now.

I agree that making Unique into something that can act as a general scope guard and take arbitrary allocation and freeing functions like std::unique_ptr may be useful, though it seems less needed than it is in C++ given that we have scope (exit).

Contributor

mrkline commented Apr 2, 2015

My rationale behind doing so was that if we remove constructors accepting RefT and promote create as the way to make a Unique, we can completely control the life cycle of the underlying object T from start to finish. This is the entire point of Unique, is it not? And if we completely control the life cycle, then there is no reason to rely on the GC. If you think that's too ambitious to start with or don't agree with the idea, I can certainly shelve it for now.

I agree that making Unique into something that can act as a general scope guard and take arbitrary allocation and freeing functions like std::unique_ptr may be useful, though it seems less needed than it is in C++ given that we have scope (exit).

@JakobOvrum

View changes

std/typecons.d
+ else
+ immutable size_t allocSize = T.sizeof;
+
+ void* rawMemory = enforce(malloc(allocSize), "malloc returned null");

This comment has been minimized.

@JakobOvrum

JakobOvrum Apr 2, 2015

Member

enforce wants to allocate to throw an exception, so you can't use it with malloc.

Do:

void* rawMemory = malloc(allocSize);
if(!rawMemory)
    onOutOfMemoryError();

(See also #3031)

@JakobOvrum

JakobOvrum Apr 2, 2015

Member

enforce wants to allocate to throw an exception, so you can't use it with malloc.

Do:

void* rawMemory = malloc(allocSize);
if(!rawMemory)
    onOutOfMemoryError();

(See also #3031)

This comment has been minimized.

@mrkline

mrkline Apr 2, 2015

Contributor

Thanks! For now I'll do this, and once both this and #3031 are hopefully merged, we can move over to that.

@mrkline

mrkline Apr 2, 2015

Contributor

Thanks! For now I'll do this, and once both this and #3031 are hopefully merged, we can move over to that.

This comment has been minimized.

@mrkline

mrkline Apr 3, 2015

Contributor

@JakobOvrum After a bit of review, I recall that I was following a pattern noticed elsewhere in Phobos:

~/s/d/phobos [v2.067.0 ?] % ag 'enforce\(malloc'
std/container/array.d
485:            auto p = enforce(malloc(sz));

std/regex/package.d
565:        _memory = (enforce(malloc(size))[0..size]);
641:            _memory = (enforce(malloc(size))[0..size]);
671:    void[] memory = enforce(malloc(size))[0..size];

std/stdio.d
346:        _p = cast(Impl*) enforce(malloc(Impl.sizeof), "Out of memory");

std/typecons.d
4053:            _store = cast(Impl*) enforce(malloc(Impl.sizeof));

std/uni.d
1745:        auto ptr = cast(T*)enforce(malloc(T.sizeof*size), "out of memory on C heap");
6875:            auto p = cast(ubyte*)enforce(malloc(raw_cap));
6917:        ubyte* p = cast(ubyte*)enforce(malloc(3*(grow+1)));

Should these be corrected in another PR?

@mrkline

mrkline Apr 3, 2015

Contributor

@JakobOvrum After a bit of review, I recall that I was following a pattern noticed elsewhere in Phobos:

~/s/d/phobos [v2.067.0 ?] % ag 'enforce\(malloc'
std/container/array.d
485:            auto p = enforce(malloc(sz));

std/regex/package.d
565:        _memory = (enforce(malloc(size))[0..size]);
641:            _memory = (enforce(malloc(size))[0..size]);
671:    void[] memory = enforce(malloc(size))[0..size];

std/stdio.d
346:        _p = cast(Impl*) enforce(malloc(Impl.sizeof), "Out of memory");

std/typecons.d
4053:            _store = cast(Impl*) enforce(malloc(Impl.sizeof));

std/uni.d
1745:        auto ptr = cast(T*)enforce(malloc(T.sizeof*size), "out of memory on C heap");
6875:            auto p = cast(ubyte*)enforce(malloc(raw_cap));
6917:        ubyte* p = cast(ubyte*)enforce(malloc(3*(grow+1)));

Should these be corrected in another PR?

This comment has been minimized.

@JakobOvrum

JakobOvrum Apr 3, 2015

Member

#3031 fixes a few of them but focuses on more important changes, but yes, we'll fix all of them eventually.

@JakobOvrum

JakobOvrum Apr 3, 2015

Member

#3031 fixes a few of them but focuses on more important changes, but yes, we'll fix all of them eventually.

mrkline added some commits Apr 3, 2015

Use class version of std.conv.emplace for Unique
I picked up the trick of converting a pointer into an array
using the [0 .. size] syntax from std/regex/package.d.

Unique.create is still segfaulting, but this seems to be an issue
with the class version of emplace regardless of this Unique work.
The bug can be found here:
https://issues.dlang.org/show_bug.cgi?id=14402
@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 3, 2015

Contributor

The segfaults I've been getting seem to be caused by a separate issue in std.conv.emplace. I opened a bug here: https://issues.dlang.org/show_bug.cgi?id=14402

Contributor

mrkline commented Apr 3, 2015

The segfaults I've been getting seem to be caused by a separate issue in std.conv.emplace. I opened a bug here: https://issues.dlang.org/show_bug.cgi?id=14402

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 4, 2015

This is the entire point of Unique, is it not?

Not entire at the very least. For me personally it is not even important point. The way I see it, best thing that can happen with Unique is integration with std.concurrency to allow sending unique mutable messages between threads without shared qualification. See also vibe.d Isolated (https://github.com/rejectedsoftware/vibe.d/blob/master/source/vibe/core/concurrency.d#L306)

It is still rather far away but I'd prefer to avoid any hard requirements about internal memory management. Though that does seem a bit out of scope of this PR so no real objections as long as you keep that in mind in the long term.

This is the entire point of Unique, is it not?

Not entire at the very least. For me personally it is not even important point. The way I see it, best thing that can happen with Unique is integration with std.concurrency to allow sending unique mutable messages between threads without shared qualification. See also vibe.d Isolated (https://github.com/rejectedsoftware/vibe.d/blob/master/source/vibe/core/concurrency.d#L306)

It is still rather far away but I'd prefer to avoid any hard requirements about internal memory management. Though that does seem a bit out of scope of this PR so no real objections as long as you keep that in mind in the long term.

Make Unique use the GC again
It is currently impossible (or so it seems) to use malloc and
emplace to create a nested class or struct, so we'll return to
using the GC for now. We'll also restore the constructors that take
a RefT, as using new _inside_ the context of the nested class or
struct is apparently the only way to create one currently.
@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 5, 2015

Contributor

Alright. So, I've done a bit of exploring and realized that context-aware objects cannot be created with emplace or new outside their context. Because of this, I've switched back to using the GC and added the constructors that take a pointer from new (since new seems to be the only valid way to currently create a context-aware object). This PR should now pass unit tests.

So we're all on the same page, this PR now makes the following changes:

  • std.algorithm.move becomes the canonical way to transfer ownership of a resource from one Unique to another. The docs and unit tests have been updated to reflect this.
  • Given the previous point, release just destroys the owned object and nulls the pointer.
  • create is no longer compiled out via version(None).
  • Releasing the owned object is now done with destroy followed by GC.free, as suggested by the "corrective action" listed for delete on the deprecated features page: http://dlang.org/deprecate.html#delete
Contributor

mrkline commented Apr 5, 2015

Alright. So, I've done a bit of exploring and realized that context-aware objects cannot be created with emplace or new outside their context. Because of this, I've switched back to using the GC and added the constructors that take a pointer from new (since new seems to be the only valid way to currently create a context-aware object). This PR should now pass unit tests.

So we're all on the same page, this PR now makes the following changes:

  • std.algorithm.move becomes the canonical way to transfer ownership of a resource from one Unique to another. The docs and unit tests have been updated to reflect this.
  • Given the previous point, release just destroys the owned object and nulls the pointer.
  • create is no longer compiled out via version(None).
  • Releasing the owned object is now done with destroy followed by GC.free, as suggested by the "corrective action" listed for delete on the deprecated features page: http://dlang.org/deprecate.html#delete
@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Apr 6, 2015

Member

Because of this, I've switched back to using the GC and added the constructors that take a pointer from new (since new seems to be the only valid way to currently create a context-aware object).

Instead of hiding this dangerous operation in an unassuming, anonymous constructor, we should probably formalize it in a named, greppable constructor function. Ideally it would be named assumeUnique... although that might be an unpopular choice due to std.exception.assumeUnique.

(See also http://acehreli.org/AliCehreli_assumptions.pdf)

Member

JakobOvrum commented Apr 6, 2015

Because of this, I've switched back to using the GC and added the constructors that take a pointer from new (since new seems to be the only valid way to currently create a context-aware object).

Instead of hiding this dangerous operation in an unassuming, anonymous constructor, we should probably formalize it in a named, greppable constructor function. Ideally it would be named assumeUnique... although that might be an unpopular choice due to std.exception.assumeUnique.

(See also http://acehreli.org/AliCehreli_assumptions.pdf)

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 6, 2015

Contributor

@JakobOvrum - I couldn't agree more. I only left those constructors because they were there already, and I was going under the assumption that fewer breaking changes would mean less debate to get this PR accepted. I'm more than happy to remove those constructors and work them into something more... excplicit in its stated intentions.

Contributor

mrkline commented Apr 6, 2015

@JakobOvrum - I couldn't agree more. I only left those constructors because they were there already, and I was going under the assumption that fewer breaking changes would mean less debate to get this PR accepted. I'm more than happy to remove those constructors and work them into something more... excplicit in its stated intentions.

@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Apr 6, 2015

Member

Alright, this is a breaking change but Unique is one of those fundamental types we need to get right. LGTM.

Member

JakobOvrum commented Apr 6, 2015

Alright, this is a breaking change but Unique is one of those fundamental types we need to get right. LGTM.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 6, 2015

Needs an entry in changelog with migration instructions in that case

Needs an entry in changelog with migration instructions in that case

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 6, 2015

Contributor

I can provide those, along with some better DDoc documentation in typecons.d itself. I'll implement the changes proposed by @JakobOvrum ASAP (hopefully tonight, possibly tomorrow).

Contributor

mrkline commented Apr 6, 2015

I can provide those, along with some better DDoc documentation in typecons.d itself. I'll implement the changes proposed by @JakobOvrum ASAP (hopefully tonight, possibly tomorrow).

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 6, 2015

btw I'd really love assumeUnique to return Unique!T instead of immutable but can't imagine good non-breaking migration path for that :(

btw I'd really love assumeUnique to return Unique!T instead of immutable but can't imagine good non-breaking migration path for that :(

Revamp and cleanup of Unique
From the related pull request
(#3139),
there seems to be a general consensus that it is more important to
do Unique "right", even if that means breaking changes, so long as
there is a clean migration path. With that in mind, I have made the
following additional changes:

- Instead of constructors that take a RefT, Uniques can now be
  created one of two ways: via .create or .fromNested.
  See the DDocs of both for details.

- opDot is replaced with "alias _p this". A cursorty Google search
  indicates that opDot is deprecated and that alias this is the
  preferred method. Like C++'s unique_ptr, Unique now enjoys
  pointer-like operations (such as dereferencing),
  but cannot be set to null or assigned from a different pointer
  due to opAssign and the disabled postblit constructor.

- Consequently, isEmpty has been removed. Instead, just use
  is null as you would with a pointer.

- Removal of redundant unit tests

- Various comment and unit test cleanup
@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 7, 2015

Contributor

There seems to be a general consensus that it is more important to do Unique right, even if that means breaking changes, so long as there is a clean migration path. With that in mind, I have made the following additional changes:

  • Instead of constructors that take a RefT, Uniques can now be created one of two ways: via .create or .fromNested. The latter takes a pointer fresh from new, since that seems to be the only way at time of writing to create a reference to a nested object.
  • opDot is replaced with alias _p this. A cursory Google search indicates that opDot is deprecated and that alias this is the preferred method. Like C++'s unique_ptr, Unique now enjoys pointer-like operations (such as dereferencing).
  • Consequently, isEmpty has been removed. Instead, just use is null as you would with a pointer. Since a pointer implicitly casts to a boolean value, if (myUnique) is also valid.
Contributor

mrkline commented Apr 7, 2015

There seems to be a general consensus that it is more important to do Unique right, even if that means breaking changes, so long as there is a clean migration path. With that in mind, I have made the following additional changes:

  • Instead of constructors that take a RefT, Uniques can now be created one of two ways: via .create or .fromNested. The latter takes a pointer fresh from new, since that seems to be the only way at time of writing to create a reference to a nested object.
  • opDot is replaced with alias _p this. A cursory Google search indicates that opDot is deprecated and that alias this is the preferred method. Like C++'s unique_ptr, Unique now enjoys pointer-like operations (such as dereferencing).
  • Consequently, isEmpty has been removed. Instead, just use is null as you would with a pointer. Since a pointer implicitly casts to a boolean value, if (myUnique) is also valid.
@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 10, 2015

Member

Why create? We don't have any create in Phobos. The common pattern is to use a normal constructor and maybe add a lowercase unique function for inference.

Member

MartinNowak commented Apr 10, 2015

Why create? We don't have any create in Phobos. The common pattern is to use a normal constructor and maybe add a lowercase unique function for inference.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 10, 2015

Member

I just made a related patch for RefCounted, #3171.

Member

MartinNowak commented Apr 10, 2015

I just made a related patch for RefCounted, #3171.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 10, 2015

Member

But assumeUnique does something very different, it casts to immutable. It was named after the precondition for converting something to immutable.

Member

MartinNowak commented Apr 10, 2015

But assumeUnique does something very different, it casts to immutable. It was named after the precondition for converting something to immutable.

@MartinNowak

View changes

std/typecons.d
+
+ To ensure uniqueness, be sure to provide the direct result of $(D new).
+
+ Example:

This comment has been minimized.

@MartinNowak

MartinNowak Apr 10, 2015

Member

You can make this example a documented unittest.

@MartinNowak

MartinNowak Apr 10, 2015

Member

You can make this example a documented unittest.

This comment has been minimized.

This comment has been minimized.

@MartinNowak

MartinNowak Apr 10, 2015

Member

OK, but if you make it a documented unittest, you don't have to copy the code.

@MartinNowak

MartinNowak Apr 10, 2015

Member

OK, but if you make it a documented unittest, you don't have to copy the code.

This comment has been minimized.

@mrkline

mrkline Apr 10, 2015

Contributor

Oh, fantastic! I wasn't aware of this feature. Thanks much.

@mrkline

mrkline Apr 10, 2015

Contributor

Oh, fantastic! I wasn't aware of this feature. Thanks much.

@MartinNowak

View changes

std/typecons.d
+ /**
+ Returns the underlying $(D RefT) for use by non-owning code.
+
+ The holder of a $(D Unique!T) is the <em>owner</em> of that $(D T).

This comment has been minimized.

@MartinNowak

MartinNowak Apr 10, 2015

Member

There is $(I owner) for italics I think, we also build a latex PDF from that docs, so HTML shouldn't be used.

@MartinNowak

MartinNowak Apr 10, 2015

Member

There is $(I owner) for italics I think, we also build a latex PDF from that docs, so HTML shouldn't be used.

This comment has been minimized.

@mrkline

mrkline Apr 10, 2015

Contributor

Thanks - I'll clean that up after work.

@mrkline

mrkline Apr 10, 2015

Contributor

Thanks - I'll clean that up after work.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 10, 2015

But assumeUnique does something very different, it casts to immutable

Which is exactly my main problem with assumeUnique. It is quite often to have uniqueness axiom stated much earlier in code than decision about mutability of result can be made. Compiler can nicely infer it for pure functions without using assumeUnique at all but in general case it is unsolved problem.

But assumeUnique does something very different, it casts to immutable

Which is exactly my main problem with assumeUnique. It is quite often to have uniqueness axiom stated much earlier in code than decision about mutability of result can be made. Compiler can nicely infer it for pure functions without using assumeUnique at all but in general case it is unsolved problem.

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 10, 2015

Contributor

I used create because the function was already there. If you would like me to change that to a constructor, I'm happy to do so.

As @Dicebot discussed, I'm not touching assumeUnique with this PR. It's a pretty bad misnomer given that it doesn't return a Unique, but changing it would open a huge can of worms.

Contributor

mrkline commented Apr 10, 2015

I used create because the function was already there. If you would like me to change that to a constructor, I'm happy to do so.

As @Dicebot discussed, I'm not touching assumeUnique with this PR. It's a pretty bad misnomer given that it doesn't return a Unique, but changing it would open a huge can of worms.

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 10, 2015

Contributor

@MartinNowak A thought just occurred to me - create exists because it allows us to build a Unique for an underlying T whose constructor takes no arguments, no?

Contributor

mrkline commented Apr 10, 2015

@MartinNowak A thought just occurred to me - create exists because it allows us to build a Unique for an underlying T whose constructor takes no arguments, no?

@MartinNowak

View changes

std/typecons.d
+ Allows you to dereference the underlying $(D RefT)
+ and treat it like a $(D RefT) in other ways (such as comparing to null)
+ */
+ alias _p this;

This comment has been minimized.

@MartinNowak

MartinNowak Apr 12, 2015

Member

Both of those functions can escape references making Unique unsafe.

@MartinNowak

MartinNowak Apr 12, 2015

Member

Both of those functions can escape references making Unique unsafe.

This comment has been minimized.

@mrkline

mrkline Apr 12, 2015

Contributor

You're right, they can, and perhaps I should add a comment saying as much. However, this is not inherently bad. Unique conveys ownership, and non-owning functions shouldn't pass around smart pointers if they aren't going to play a role in the underlying object's lifetime.

See this bit (starting at the 14 minute mark or so) of a recent Herb Sutter talk. Yes, it's about C++, but the exact same semantics are at play here and he discusses it extensively. He makes these points much more eloquently than I do, so I hope you take the time to watch it, but to summarize, doing this bakes the ownership semantics of calls directly into the function signatures. Observe:

void consume(Unique!T u) // Takes ownership of u

void poke(ref T u) // Uses u but plays no impact on its lifetime

void opt(T* u) // u is optional; has no impact on its lifetime

If you're still not convinced, another important point is that a non-owning function shouldn't care how ownership of an object is being handled.

void antipatternIfNotOwning(ref Unique!T u)

is just fine to tell the user "this function may reseat the ownership of the T". But it's just bad if the function doesn't affect ownership. It demands a certain means of ownership (as opposed to GC, or RefCounted, or stack-allocated) despite not actually caring, it's more verbose, and its intentions aren't as clear.

@mrkline

mrkline Apr 12, 2015

Contributor

You're right, they can, and perhaps I should add a comment saying as much. However, this is not inherently bad. Unique conveys ownership, and non-owning functions shouldn't pass around smart pointers if they aren't going to play a role in the underlying object's lifetime.

See this bit (starting at the 14 minute mark or so) of a recent Herb Sutter talk. Yes, it's about C++, but the exact same semantics are at play here and he discusses it extensively. He makes these points much more eloquently than I do, so I hope you take the time to watch it, but to summarize, doing this bakes the ownership semantics of calls directly into the function signatures. Observe:

void consume(Unique!T u) // Takes ownership of u

void poke(ref T u) // Uses u but plays no impact on its lifetime

void opt(T* u) // u is optional; has no impact on its lifetime

If you're still not convinced, another important point is that a non-owning function shouldn't care how ownership of an object is being handled.

void antipatternIfNotOwning(ref Unique!T u)

is just fine to tell the user "this function may reseat the ownership of the T". But it's just bad if the function doesn't affect ownership. It demands a certain means of ownership (as opposed to GC, or RefCounted, or stack-allocated) despite not actually caring, it's more verbose, and its intentions aren't as clear.

This comment has been minimized.

@mihails-strasuns

mihails-strasuns Apr 12, 2015

C++ is hardly a decent example to follow. The stuff they are forced to use here is all about using convention in absence of working compile-verified solution. I believe D can and should do better in this regard.

Allowing @safe escaping of unique reference is absolutely unacceptable.

@mihails-strasuns

mihails-strasuns Apr 12, 2015

C++ is hardly a decent example to follow. The stuff they are forced to use here is all about using convention in absence of working compile-verified solution. I believe D can and should do better in this regard.

Allowing @safe escaping of unique reference is absolutely unacceptable.

This comment has been minimized.

@mrkline

mrkline Apr 12, 2015

Contributor

Did you take the time to watch the linked section of the talk? D can (unlike C++) do compile-time checks for safety using @safe, but since it can't verify life cycles at compile time (a la Rust), that doesn't get around the inherent ownership issues at hand.

If we want to mark this as unsafe, that's fine, but I still strongly believe it's necessary. What's the alternative? Passing ref Unique!T around? That's problematic for the reasons listed above.

@mrkline

mrkline Apr 12, 2015

Contributor

Did you take the time to watch the linked section of the talk? D can (unlike C++) do compile-time checks for safety using @safe, but since it can't verify life cycles at compile time (a la Rust), that doesn't get around the inherent ownership issues at hand.

If we want to mark this as unsafe, that's fine, but I still strongly believe it's necessary. What's the alternative? Passing ref Unique!T around? That's problematic for the reasons listed above.

This comment has been minimized.

@mihails-strasuns

mihails-strasuns Apr 13, 2015

Yes and I have been using that approach in C++ for ages. It is different in D though as we actually promise @safe code to be always memory-safe if it compiles. No conventions, no style rules - if it compiles, it must be good to go. This can't be compromised no matter what, even simply banning all borrowing of Unique would be more practical approach.

And making Unique @system is just delaying another full rewrite of the utility.

Also please note there are actually various proposals about enforcing borrowing / lifetime semantics at compile-time in a way similar to Rust, with http://wiki.dlang.org/DIP69 being primary candidate for getting implemented. I haven't checked it in a while but it should allow adding scope ref T borrow() { return _p; } method while keeping it all @safe.

@mihails-strasuns

mihails-strasuns Apr 13, 2015

Yes and I have been using that approach in C++ for ages. It is different in D though as we actually promise @safe code to be always memory-safe if it compiles. No conventions, no style rules - if it compiles, it must be good to go. This can't be compromised no matter what, even simply banning all borrowing of Unique would be more practical approach.

And making Unique @system is just delaying another full rewrite of the utility.

Also please note there are actually various proposals about enforcing borrowing / lifetime semantics at compile-time in a way similar to Rust, with http://wiki.dlang.org/DIP69 being primary candidate for getting implemented. I haven't checked it in a while but it should allow adding scope ref T borrow() { return _p; } method while keeping it all @safe.

This comment has been minimized.

@mrkline

mrkline Apr 13, 2015

Contributor

Point taken. My apologies if my tone was somewhat... irreverent.

So, what's the plan? Drop get and just pass Uniques by ref? And what should we do about opDot? I only moved away from it because there was no documentation for it anywhere, and several posts said this was because it was deprecated for alias this.

@mrkline

mrkline Apr 13, 2015

Contributor

Point taken. My apologies if my tone was somewhat... irreverent.

So, what's the plan? Drop get and just pass Uniques by ref? And what should we do about opDot? I only moved away from it because there was no documentation for it anywhere, and several posts said this was because it was deprecated for alias this.

This comment has been minimized.

@MartinNowak

MartinNowak Apr 13, 2015

Member

What Dicevot says. I know that talk btw.

@MartinNowak

MartinNowak Apr 13, 2015

Member

What Dicevot says. I know that talk btw.

This comment has been minimized.

@mihails-strasuns

mihails-strasuns Apr 13, 2015

No offense, I simply wanted to stress the point how important guarantess of @safe are.

Another alternative for opDot is using std.typecons.Proxy : https://github.com/D-Programming-Language/phobos/blob/master/std/typecons.d#L4821-L4829
I don't know how robust it is in practice but is supposed to provide wrappers for all members without allowing implicit conversion (like alias this does).

As for get - yes, I am tempted to just drop it completely for now and add better facility once DIP69 or similar is implemented. But this is a delicate topic, I wonder what Phobos maintainers think about it.

@mihails-strasuns

mihails-strasuns Apr 13, 2015

No offense, I simply wanted to stress the point how important guarantess of @safe are.

Another alternative for opDot is using std.typecons.Proxy : https://github.com/D-Programming-Language/phobos/blob/master/std/typecons.d#L4821-L4829
I don't know how robust it is in practice but is supposed to provide wrappers for all members without allowing implicit conversion (like alias this does).

As for get - yes, I am tempted to just drop it completely for now and add better facility once DIP69 or similar is implemented. But this is a delicate topic, I wonder what Phobos maintainers think about it.

This comment has been minimized.

@MartinNowak

MartinNowak Apr 13, 2015

Member

Looking at DIP25 we should implement this like so.

ref T get() return { return *_p; }
alias get this;

That should be safe.

@MartinNowak

MartinNowak Apr 13, 2015

Member

Looking at DIP25 we should implement this like so.

ref T get() return { return *_p; }
alias get this;

That should be safe.

This comment has been minimized.

@MartinNowak

View changes

std/typecons.d
+ import core.memory : GC;
+
+ destroy(_p);
+ GC.free(cast(void*)_p);

This comment has been minimized.

@MartinNowak

MartinNowak Apr 12, 2015

Member

Why would Unique require T to be a GC allocated object?

@MartinNowak

MartinNowak Apr 12, 2015

Member

Why would Unique require T to be a GC allocated object?

This comment has been minimized.

@mrkline

mrkline Apr 12, 2015

Contributor

If you look at the commit history of this PR, you will see that I originally tried using malloc and free, but things fell apart when it came to non-static, nested classes and structs (ones that required a closure). emplace, in its current form, just doesn't work for them. See https://issues.dlang.org/show_bug.cgi?id=14402 and the comments I've already made on this topic above.

If I'm wrong and you can initialize types requiring a closure using something besides new, please let me know, but I couldn't figure out any way to get them properly initialized with the frame pointer of their context.

@mrkline

mrkline Apr 12, 2015

Contributor

If you look at the commit history of this PR, you will see that I originally tried using malloc and free, but things fell apart when it came to non-static, nested classes and structs (ones that required a closure). emplace, in its current form, just doesn't work for them. See https://issues.dlang.org/show_bug.cgi?id=14402 and the comments I've already made on this topic above.

If I'm wrong and you can initialize types requiring a closure using something besides new, please let me know, but I couldn't figure out any way to get them properly initialized with the frame pointer of their context.

This comment has been minimized.

@MartinNowak

MartinNowak Apr 13, 2015

Member

If that's also true for RefCounted we should disable such types for now and find a proper solution.
Note that it possible though hacky to set the context pointer, the problem is more how to get the right context ptr. For nested struct you should be able to first initialize a normal value (compiler will fill the context ptr), then move that value. For nested classes you could have a separate argument.

@MartinNowak

MartinNowak Apr 13, 2015

Member

If that's also true for RefCounted we should disable such types for now and find a proper solution.
Note that it possible though hacky to set the context pointer, the problem is more how to get the right context ptr. For nested struct you should be able to first initialize a normal value (compiler will fill the context ptr), then move that value. For nested classes you could have a separate argument.

This comment has been minimized.

@rsw0x

rsw0x Apr 13, 2015

Contributor

I think it's extremely important for Unique!T to be able to infer @nogc, or else its value diminishes greatly - so this should probably be done as malloc/free and the emplace issues should be fixed instead of having to use the GC, no?

@rsw0x

rsw0x Apr 13, 2015

Contributor

I think it's extremely important for Unique!T to be able to infer @nogc, or else its value diminishes greatly - so this should probably be done as malloc/free and the emplace issues should be fixed instead of having to use the GC, no?

This comment has been minimized.

@mrkline

mrkline Apr 13, 2015

Contributor

From my experimentation in picking apart emplace, the startup sequence would have to be:

  1. Blit the allocated memory with typeid(T).init
  2. Somehow assign the frame/context pointer
  3. Call the constructor (Doing so without step 2, as emplace currently does, causes segfaults when a constructor tries to reach into the context via a null frame pointer.)

I'm all for that option, but how can it be done? @MartinNowak - you mention some way to set the context pointer - could you show this with a small code sample? How does one get the address of their current frame?

@mrkline

mrkline Apr 13, 2015

Contributor

From my experimentation in picking apart emplace, the startup sequence would have to be:

  1. Blit the allocated memory with typeid(T).init
  2. Somehow assign the frame/context pointer
  3. Call the constructor (Doing so without step 2, as emplace currently does, causes segfaults when a constructor tries to reach into the context via a null frame pointer.)

I'm all for that option, but how can it be done? @MartinNowak - you mention some way to set the context pointer - could you show this with a small code sample? How does one get the address of their current frame?

This comment has been minimized.

@MartinNowak

MartinNowak Apr 14, 2015

Member

Only the compiler can get the context pointer, and it will only do so when you initialize a nested struct (the normal way, not by emplacing).

unique(T, Args...)(Args args)
{
   auto tmp = T(args); // will init context pointer
   moveEmplace(storage, tmp);
}
@MartinNowak

MartinNowak Apr 14, 2015

Member

Only the compiler can get the context pointer, and it will only do so when you initialize a nested struct (the normal way, not by emplacing).

unique(T, Args...)(Args args)
{
   auto tmp = T(args); // will init context pointer
   moveEmplace(storage, tmp);
}

This comment has been minimized.

@mrkline

mrkline Apr 17, 2015

Contributor

One cannot initialize a nested struct that way, so I guess they're out too.

void foo(T, A...)(auto ref A args)
{
    import std.algorithm : move;
    auto tmp = T(args); // would init context pointer if it didn't crash
    auto other = move(tmp);
}

void main()
{
    int destroyed;
    struct Nested
    {
        ~this() { ++destroyed; }
    }
    foo!Nested();
    assert(destroyed == 1);
}
$ rdmd /tmp/nope.d
/tmp/nope.d(4): Error: cannot access frame pointer of nope.main.Nested
/tmp/nope.d(15): Error: template instance nope.foo!(Nested, ) error instantiating
@mrkline

mrkline Apr 17, 2015

Contributor

One cannot initialize a nested struct that way, so I guess they're out too.

void foo(T, A...)(auto ref A args)
{
    import std.algorithm : move;
    auto tmp = T(args); // would init context pointer if it didn't crash
    auto other = move(tmp);
}

void main()
{
    int destroyed;
    struct Nested
    {
        ~this() { ++destroyed; }
    }
    foo!Nested();
    assert(destroyed == 1);
}
$ rdmd /tmp/nope.d
/tmp/nope.d(4): Error: cannot access frame pointer of nope.main.Nested
/tmp/nope.d(15): Error: template instance nope.foo!(Nested, ) error instantiating

This comment has been minimized.

@MartinNowak

MartinNowak Apr 17, 2015

Member

Ah OK, I thought that would make the foo instantiation local, as we do for nested functions.
You could construct the value on the caller side unique(T(args)).

@MartinNowak

MartinNowak Apr 17, 2015

Member

Ah OK, I thought that would make the foo instantiation local, as we do for nested functions.
You could construct the value on the caller side unique(T(args)).

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 12, 2015

Member

Making this a freestanding unique function would be the right move.

Member

MartinNowak commented Apr 12, 2015

Making this a freestanding unique function would be the right move.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 12, 2015

Member

Let's spend a little more time on this, Unique was basically unusable before, so we have a chance to fix it and should do this thoroughly.

I think Unique should use the same storage as RefCounted, i.e. malloc/free and it should be constructible using unique!T(args), unique!T(), and unique(move(t)).

Member

MartinNowak commented Apr 12, 2015

Let's spend a little more time on this, Unique was basically unusable before, so we have a chance to fix it and should do this thoroughly.

I think Unique should use the same storage as RefCounted, i.e. malloc/free and it should be constructible using unique!T(args), unique!T(), and unique(move(t)).

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 12, 2015

Contributor

Let's take all the time we need. Since this is a fairly fundamental type, and because it's mostly unusable now, I think we all agree that we should take the time to get it done right.

Sorry for not getting to the cleanup and making the unit tests documented as we discussed above. With luck I should get to it tonight.

Contributor

mrkline commented Apr 12, 2015

Let's take all the time we need. Since this is a fairly fundamental type, and because it's mostly unusable now, I think we all agree that we should take the time to get it done right.

Sorry for not getting to the cleanup and making the unit tests documented as we discussed above. With luck I should get to it tonight.

Turn Unique.create into a freestanding unique()
Also cleaned up the Unique documentation a bit.
Unfortunately some of the unittests could not be used as documented
unittests because placing them in the struct gives an error about
RTInfo!(Nested) being recursvely expanded.
@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 13, 2015

Contributor

I've turned Unique.create into unique as requested by @MartinNowak, polished up the docs slightly, and updated the PR description.

Contributor

mrkline commented Apr 13, 2015

I've turned Unique.create into unique as requested by @MartinNowak, polished up the docs slightly, and updated the PR description.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 13, 2015

Member

No worries, let's try to get @WalterBright @schuetzm on board to advise what to do about the escaping problems.
FWIW we should implement them as sealed references.

Member

MartinNowak commented Apr 13, 2015

No worries, let's try to get @WalterBright @schuetzm on board to advise what to do about the escaping problems.
FWIW we should implement them as sealed references.

@@ -64,67 +69,44 @@ static if (is(T:Object))
else
alias RefT = T*;

This comment has been minimized.

@MartinNowak

MartinNowak Apr 13, 2015

Member

We should probably choose scoped!T as storage for classes, what does RefCounted do here?

@MartinNowak

MartinNowak Apr 13, 2015

Member

We should probably choose scoped!T as storage for classes, what does RefCounted do here?

This comment has been minimized.

@rsw0x

rsw0x Apr 13, 2015

Contributor

RefCounted doesn't support classes. https://issues.dlang.org/show_bug.cgi?id=14168

@rsw0x

rsw0x Apr 13, 2015

Contributor

RefCounted doesn't support classes. https://issues.dlang.org/show_bug.cgi?id=14168

This comment has been minimized.

@mrkline

mrkline Apr 13, 2015

Contributor

Exactly. And scoped!T doesn't work for nested classes because it uses emplace, and AFAIK the only way to construct a nested type is with new, in that scope (see our previous conversation about this).

Once we figure out what we want to do here with Unique, I'll open another PR to give RefCounted a similar treatment.

@mrkline

mrkline Apr 13, 2015

Contributor

Exactly. And scoped!T doesn't work for nested classes because it uses emplace, and AFAIK the only way to construct a nested type is with new, in that scope (see our previous conversation about this).

Once we figure out what we want to do here with Unique, I'll open another PR to give RefCounted a similar treatment.

This comment has been minimized.

@MartinNowak

MartinNowak Apr 14, 2015

Member

I think we'll figure out a solution for classes and nested classes as well. Let's take them out of the calculation for now. And yes RefCounted should support classes as well, but we'll have to coordinate this with the RC class DIP proposal.

@MartinNowak

MartinNowak Apr 14, 2015

Member

I think we'll figure out a solution for classes and nested classes as well. Let's take them out of the calculation for now. And yes RefCounted should support classes as well, but we'll have to coordinate this with the RC class DIP proposal.

@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Apr 13, 2015

Contributor

I think an extension of DIP25 (sealed references) is more likely to be the final solution than DIP69; your return ref suggestion is therefore the way to go.

Coincidentally, I dumped my thoughts on unique/isolated into the wiki yesterday:
http://wiki.dlang.org/User:Schuetzm/isolated

I came across an interesting difference between Unique and deadalnix' @isolated idea: Unique is not transitive, but @isolated needs to be, otherwise it couldn't be converted to immutable or shared. Not sure yet how to reconcile the two.

Contributor

schuetzm commented Apr 13, 2015

I think an extension of DIP25 (sealed references) is more likely to be the final solution than DIP69; your return ref suggestion is therefore the way to go.

Coincidentally, I dumped my thoughts on unique/isolated into the wiki yesterday:
http://wiki.dlang.org/User:Schuetzm/isolated

I came across an interesting difference between Unique and deadalnix' @isolated idea: Unique is not transitive, but @isolated needs to be, otherwise it couldn't be converted to immutable or shared. Not sure yet how to reconcile the two.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 13, 2015

Hm, now that you have reminded me of non-transitivity of DIP25, how will this work?

struct Payload
{
    private ubyte* stuff;
    ref ubyte opIndex(size_t index) return { return stuff[index]; }
    this(size_t size) { stuff = malloc(size); }
    ~this() { free(stuff); }
}

// ...
auto owner = unique!Payload(42);
ubyte* ptr = &(owner.get[10]); // will this be rejected assuming get is also return ref?

Hm, now that you have reminded me of non-transitivity of DIP25, how will this work?

struct Payload
{
    private ubyte* stuff;
    ref ubyte opIndex(size_t index) return { return stuff[index]; }
    this(size_t size) { stuff = malloc(size); }
    ~this() { free(stuff); }
}

// ...
auto owner = unique!Payload(42);
ubyte* ptr = &(owner.get[10]); // will this be rejected assuming get is also return ref?
@schuetzm

This comment has been minimized.

Show comment
Hide comment
@schuetzm

schuetzm Apr 13, 2015

Contributor

Compilable example (DMD master), but without pointers:

import std.c.stdlib;

struct Payload
{
    private ubyte* stuff;
    ref ubyte opIndex(size_t index) return { return stuff[index]; }
    this(size_t size) { stuff = cast(ubyte*) malloc(size); }
    ~this() { free(stuff); }
}

struct Unique(T) {
    private T payload;
    ref T get() return { return payload; }
}

auto unique(T, Args...)(Args args) {
    return Unique!T(T(args));
}

ref test() {
    auto owner = unique!Payload(42);
    return owner.get[10];
}
# dmd -c -dip25 xx.d
xx.d(23): Error: escaping reference to local variable owner

It doesn't really work yet when pointers are involved, because DIP25 only handles ref. This also isn't an example of transitivity; it only comes into play when you have a double reference, like int** or ref int*. My proposal handles those conservatively: on reading, the lifetime is assume to be that of the outer reference, on writing, it's assumed to be infinite.

Contributor

schuetzm commented Apr 13, 2015

Compilable example (DMD master), but without pointers:

import std.c.stdlib;

struct Payload
{
    private ubyte* stuff;
    ref ubyte opIndex(size_t index) return { return stuff[index]; }
    this(size_t size) { stuff = cast(ubyte*) malloc(size); }
    ~this() { free(stuff); }
}

struct Unique(T) {
    private T payload;
    ref T get() return { return payload; }
}

auto unique(T, Args...)(Args args) {
    return Unique!T(T(args));
}

ref test() {
    auto owner = unique!Payload(42);
    return owner.get[10];
}
# dmd -c -dip25 xx.d
xx.d(23): Error: escaping reference to local variable owner

It doesn't really work yet when pointers are involved, because DIP25 only handles ref. This also isn't an example of transitivity; it only comes into play when you have a double reference, like int** or ref int*. My proposal handles those conservatively: on reading, the lifetime is assume to be that of the outer reference, on writing, it's assumed to be infinite.

@ntrel

This comment has been minimized.

Show comment
Hide comment
@ntrel

ntrel Apr 17, 2015

Contributor

Just saw this, using malloc/free is great work.

release just destroys the owned object and nulls the pointer

This seems like unnecessary breakage, or is there a reason for this? Why not just deprecate release? Adding the new release seems to be just a duplicate of destroy(myUnique).

this seems extremely clumsy with regards to class types, as get() now returns a reference to a reference

I think Unique!Object types shouldn't have get return by ref, and there currently isn't a way to stop the class reference escaping, we need scope e.g. DIP69.

Contributor

ntrel commented Apr 17, 2015

Just saw this, using malloc/free is great work.

release just destroys the owned object and nulls the pointer

This seems like unnecessary breakage, or is there a reason for this? Why not just deprecate release? Adding the new release seems to be just a duplicate of destroy(myUnique).

this seems extremely clumsy with regards to class types, as get() now returns a reference to a reference

I think Unique!Object types shouldn't have get return by ref, and there currently isn't a way to stop the class reference escaping, we need scope e.g. DIP69.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 17, 2015

Member

Since DIP25 handles ref types, this seems extremely clumsy with regards to class types, as get() now returns a reference to a reference (since class types are reference types).

Yes, that doesn't make sense, it would allow you to set _p to something different than the malloc'ed memory. You could wrap classes into scoped and return a ref to that, but scoped isn't yet safe either.
I think we should disallow classes until a good/common solution has been found for Unique and RefCounted.

Member

MartinNowak commented Apr 17, 2015

Since DIP25 handles ref types, this seems extremely clumsy with regards to class types, as get() now returns a reference to a reference (since class types are reference types).

Yes, that doesn't make sense, it would allow you to set _p to something different than the malloc'ed memory. You could wrap classes into scoped and return a ref to that, but scoped isn't yet safe either.
I think we should disallow classes until a good/common solution has been found for Unique and RefCounted.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 17, 2015

Member

This seems like unnecessary breakage, or is there a reason for this? Why not just deprecate release? Adding the new release seems to be just a duplicate of destroy(myUnique).

Ideally we could implement release to release the ownership of the underlying object (unsafe) and return it. But I don't see how one could reliably destroy a released value, because it requires to know the implementation of Unique, i.e. free and GC.removeRange. So we should probably deprecate that function.

The current semantics of uniq.release are equivalent to move(uniq), so deprecated("use std.algorithm.move instead") would make sense.

Member

MartinNowak commented Apr 17, 2015

This seems like unnecessary breakage, or is there a reason for this? Why not just deprecate release? Adding the new release seems to be just a duplicate of destroy(myUnique).

Ideally we could implement release to release the ownership of the underlying object (unsafe) and return it. But I don't see how one could reliably destroy a released value, because it requires to know the implementation of Unique, i.e. free and GC.removeRange. So we should probably deprecate that function.

The current semantics of uniq.release are equivalent to move(uniq), so deprecated("use std.algorithm.move instead") would make sense.

@rsw0x

This comment has been minimized.

Show comment
Hide comment
@rsw0x

rsw0x Apr 18, 2015

Contributor

re:classes,
Someone should probably ask Walter or Andrei what the status of DIP74 is, as that seems to directly relate to this/overlap(?).

Contributor

rsw0x commented Apr 18, 2015

re:classes,
Someone should probably ask Walter or Andrei what the status of DIP74 is, as that seems to directly relate to this/overlap(?).

@ntrel

This comment has been minimized.

Show comment
Hide comment
@ntrel

ntrel Apr 18, 2015

Contributor

I think we should disallow classes until a good/common solution has been found

Personally I would allow classes (without ref) but note in the docs that scope enforcement will be added later. If we stop supporting classes now there would be no upgrade path for correct, safe code already using Unique!Class. We can't know how much code uses that, but it seems an important principle to not break working code without providing an equivalent alternative. (Breaking Unique!T(new T) is justifiable breakage though to allow @nogc).

edit: Making Unique!Class.get @system should be OK.

Contributor

ntrel commented Apr 18, 2015

I think we should disallow classes until a good/common solution has been found

Personally I would allow classes (without ref) but note in the docs that scope enforcement will be added later. If we stop supporting classes now there would be no upgrade path for correct, safe code already using Unique!Class. We can't know how much code uses that, but it seems an important principle to not break working code without providing an equivalent alternative. (Breaking Unique!T(new T) is justifiable breakage though to allow @nogc).

edit: Making Unique!Class.get @system should be OK.

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 19, 2015

Contributor
  1. You're right, destroy can do the job of release, and I should just put it back to what it did originally and deprecate it.
  2. I'm strongly with @ntrel here. Saying "sorry, classes aren't supported yet until some unspecified time in the future when we offer better safety" is a bad idea. We should focus on getting a good interface now and adding safety as the language evolves to support it. That way, there won't be changes people need to migrate to in the future, and users who are already using it safely will, without code changes, start getting additional safety as it is introduced to the language.
  3. As everyone else has said, I would be incredibly curious as to what @andralex and @WalterBright have to say about this.
Contributor

mrkline commented Apr 19, 2015

  1. You're right, destroy can do the job of release, and I should just put it back to what it did originally and deprecate it.
  2. I'm strongly with @ntrel here. Saying "sorry, classes aren't supported yet until some unspecified time in the future when we offer better safety" is a bad idea. We should focus on getting a good interface now and adding safety as the language evolves to support it. That way, there won't be changes people need to migrate to in the future, and users who are already using it safely will, without code changes, start getting additional safety as it is introduced to the language.
  3. As everyone else has said, I would be incredibly curious as to what @andralex and @WalterBright have to say about this.
@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 20, 2015

Member

Then it should be T get() @system for classes, until we can control escaping.

Member

MartinNowak commented Apr 20, 2015

Then it should be T get() @system for classes, until we can control escaping.

@MartinNowak

View changes

std/typecons.d
+ import std.algorithm : move;
+ auto temp = T(args);
+ u._p = cast(T*)rawMemory;
+ *u._p = move(temp);

This comment has been minimized.

@MartinNowak

MartinNowak Apr 20, 2015

Member

That doesn't work like that, because you're assigning to uninitialized memory, see RefCounted.move for how to do moveEmplace.

@MartinNowak

MartinNowak Apr 20, 2015

Member

That doesn't work like that, because you're assigning to uninitialized memory, see RefCounted.move for how to do moveEmplace.

This comment has been minimized.

@mrkline

mrkline Apr 21, 2015

Contributor

I'll go back to just using emplace as I was originally. There's no benefit from creating a struct and then moving it in as opposed to just calling emplace AFAIK.

@mrkline

mrkline Apr 21, 2015

Contributor

I'll go back to just using emplace as I was originally. There's no benefit from creating a struct and then moving it in as opposed to just calling emplace AFAIK.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 20, 2015

Member

We could make Unique and RefCounted use the same internal storage struct, to encapsulate common construct/move/memory code.

Member

MartinNowak commented Apr 20, 2015

We could make Unique and RefCounted use the same internal storage struct, to encapsulate common construct/move/memory code.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 20, 2015

Yeah making get() @system for classes and @safe for value types makes sense

Yeah making get() @system for classes and @safe for value types makes sense

@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Apr 21, 2015

Member

It's not an issue of classes vs anything else, it's an issue of types with indirection.

Member

JakobOvrum commented Apr 21, 2015

It's not an issue of classes vs anything else, it's an issue of types with indirection.

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Apr 21, 2015

@JakobOvrum is it? Indirection on its own should be fine as long as that data is @safe on its own. Issue with classes is related to their inherent reference semantics which are not covered by DIP25 (it requires usage of ref)

@JakobOvrum is it? Indirection on its own should be fine as long as that data is @safe on its own. Issue with classes is related to their inherent reference semantics which are not covered by DIP25 (it requires usage of ref)

@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Apr 22, 2015

Member

@Dicebot, right, I don't think I've wrapped my head around DIP25 completely yet.


Regarding C heap vs GC heap; if we want to allow transferring Unique!T (where T is unshared and mutable/const) to other threads, using the C heap might be a big advantage, as there are GC improvement ideas floating around involving thread-local GC heaps (@deadalnix?). It's best if we can decide the sharedness of a GC-allocated chunk at allocation-time.

Member

JakobOvrum commented Apr 22, 2015

@Dicebot, right, I don't think I've wrapped my head around DIP25 completely yet.


Regarding C heap vs GC heap; if we want to allow transferring Unique!T (where T is unshared and mutable/const) to other threads, using the C heap might be a big advantage, as there are GC improvement ideas floating around involving thread-local GC heaps (@deadalnix?). It's best if we can decide the sharedness of a GC-allocated chunk at allocation-time.

@JakobOvrum

This comment has been minimized.

Show comment
Hide comment
@JakobOvrum

JakobOvrum Apr 23, 2015

Member

Another observation regarding cross-thread moves - the uniqueness of Unique!T only applies to T, not references embedded as T's fields. That is, the uniqueness is not transitive, it only applies for one level of indirection - the Unique!T reference itself (as has been observed by others before me).

That means not just any Unique instance can be moved to a different thread, only instances where all of T's fields passes isUnique!U || !hasUnsharedAliasing!U, where U is the type of the field.

Does this make sense?

Member

JakobOvrum commented Apr 23, 2015

Another observation regarding cross-thread moves - the uniqueness of Unique!T only applies to T, not references embedded as T's fields. That is, the uniqueness is not transitive, it only applies for one level of indirection - the Unique!T reference itself (as has been observed by others before me).

That means not just any Unique instance can be moved to a different thread, only instances where all of T's fields passes isUnique!U || !hasUnsharedAliasing!U, where U is the type of the field.

Does this make sense?

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 23, 2015

Member

Can we please keep that highly speculative GC discussion out of here?
The idea of using shared to separate thread-local from global allocations is pretty old and has many pitfalls.

That means not just any Unique instance can be moved to a different thread, only instances where all of T's fields passes isUnique!U || !hasUnsharedAliasing!U, where U is the type of the field.

Makes sense, and once we merge this we should make std.concurrent aware of Unique.

Member

MartinNowak commented Apr 23, 2015

Can we please keep that highly speculative GC discussion out of here?
The idea of using shared to separate thread-local from global allocations is pretty old and has many pitfalls.

That means not just any Unique instance can be moved to a different thread, only instances where all of T's fields passes isUnique!U || !hasUnsharedAliasing!U, where U is the type of the field.

Makes sense, and once we merge this we should make std.concurrent aware of Unique.

- Unique!C uc = new C;
- Unique!Object uo = uc.release;
+ Unique!C uc = unique!C();
+ Unique!Object uo = move(uc);

This comment has been minimized.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Don't think that polymorph conversion works, should turn this into a documented unittest.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Don't think that polymorph conversion works, should turn this into a documented unittest.

This comment has been minimized.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Oh, it's actually is supposed to work, nice.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Oh, it's actually is supposed to work, nice.

@MartinNowak

View changes

std/typecons.d
+ bool opCast(T : bool)() const { return !empty; }
+
+ /**
+ Allows you to reference the underlying $(D RefT)

This comment has been minimized.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Forwards to the underlying value.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Forwards to the underlying value.

@MartinNowak

View changes

std/typecons.d
+ return _p;
+ }
+
+ @property bool empty() const

This comment has been minimized.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Missing docs.

@MartinNowak

MartinNowak Apr 23, 2015

Member

Missing docs.

@MartinNowak

View changes

std/typecons.d
+ return _p is null;
+ }
+
+ bool opCast(T : bool)() const { return !empty; }

This comment has been minimized.

+ args = Arguments to pass to $(D T)'s constructor.
+
+*/
+Unique!T unique(T, A...)(auto ref A args)

This comment has been minimized.

@MartinNowak

MartinNowak Apr 23, 2015

Member

We should also add a move constructor for unique!T(T()), that is when you move a complete T value into unique.

@MartinNowak

MartinNowak Apr 23, 2015

Member

We should also add a move constructor for unique!T(T()), that is when you move a complete T value into unique.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 23, 2015

Member

Looks pretty good already, just a few more details.

Member

MartinNowak commented Apr 23, 2015

Looks pretty good already, just a few more details.

@mrkline

This comment has been minimized.

Show comment
Hide comment
@mrkline

mrkline Apr 24, 2015

Contributor

Alright, I added the suggested docs and made get a template function (seemed like the simplest solution). I agree that a move constructor for a T rvalue would be nice, but perhaps we can save that for another PR that also lifts your moveEmplace functionality from #3171 into its own function.

Contributor

mrkline commented Apr 24, 2015

Alright, I added the suggested docs and made get a template function (seemed like the simplest solution). I agree that a move constructor for a T rvalue would be nice, but perhaps we can save that for another PR that also lifts your moveEmplace functionality from #3171 into its own function.

Elaborate on nested types and Unique
They don't go well together.
@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 24, 2015

Member

Auto-merge toggled on

Member

MartinNowak commented Apr 24, 2015

Auto-merge toggled on

MartinNowak added a commit that referenced this pull request Apr 24, 2015

@MartinNowak MartinNowak merged commit 8f4a85b into dlang:master Apr 24, 2015

1 check passed

auto-tester Pass: 10
Details
@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Apr 24, 2015

Member

Thanks, I'll follow up with RefCounted support for classes and factoring out moveEmplace.

Member

MartinNowak commented Apr 24, 2015

Thanks, I'll follow up with RefCounted support for classes and factoring out moveEmplace.

@MartinNowak MartinNowak added this to the 2.068 milestone Jun 30, 2015

MartinNowak added a commit to MartinNowak/phobos that referenced this pull request Jul 21, 2015

Revert "Merge pull request #3139 from mrkline/better-unique"
This reverts commit 8f4a85b, reversing
changes made to d74e4d7.

Delay unfinished feature until after 2.068.x.

quickfur added a commit that referenced this pull request Jul 21, 2015

Merge pull request #3506 from MartinNowak/stable
Revert "Merge pull request #3139 from mrkline/better-unique"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment