RefCounted implementation for classes #3259

MartinNowak · 2015-05-06T01:22:10Z

add a simple RefCounted variant for classes
varies quite a lot from the RefCounted for structs
because certain semantics aren't possible with classes
also remove the autoInit stuff and use a function+@disable
this() to guarantee that the payload is initialized

MartinNowak · 2015-05-06T01:25:38Z

This still needs a bit polishing (docs et.al.), but I wanted to discuss a few things upfront.
The implementation for classes varies quite a lot from the existing RefCounted (which is also a PITA due to the autoInit stuff), but as things like autoInit and opAssign aren't implementable for classes and most of the other functions differ, I just made a second RefCounted.

HK47196 · 2015-05-06T04:04:02Z

Could you add an @nogc unit test(AFAICT this can infer @nogc, right?)

JakobOvrum · 2015-05-06T06:41:57Z

I agree this doesn't need the lazy initialization stuff, but instead of making it non-nullable, why not make it work just like any class reference T and be null by default? It should make it easier to use, usable in generic code that needs default-initialization, and the user should be able to make it non-null by composing it with a NotNull type.

It shouldn't be hard to implement; just cast _store instead of _store.payload (although either would probably work even when _store is null) and then add the proper overloads for typeof(null).

MartinNowak · 2015-05-06T07:05:51Z

Added the @nogc test, adjusted the methods of the old RefCounted, and enabled ref escape checking for that.

why not make it work just like any class reference T and be null by default

Because it's a class value, not a class reference, i.e. you can't nullify scoped!Class either.
Granted it's possible to special case null, but being able to do rc = null; seems weird when you can't do rc = cinst;.
How would you initialize a null RefCounted!T?

TODO:

implement polymorphic RefCounted conversions

MartinNowak · 2015-05-06T07:24:40Z

In fact using payload and refCount might hide members of the payload, will change that back to the old scheme of wrapping everything in refCountedStore.

JakobOvrum · 2015-05-06T07:35:17Z

Because it's a class value, not a class reference, i.e. you can't nullify scoped!Class either.

RefCounted!T is a reference by any account I can think of. It is implemented in terms of a single pointer, and copying it makes an alias, not a copy of the payload. Ref is right there in the name.

Granted it's possible to special case null, but being able to do rc = null; seems weird when you can't do rc = cinst;.

You can if cinst is also a RefCounted.

How would you initialize a null RefCounted!T?

Analogous to T:

// These are equivalent
RefCounted!T c;
RefCounted!T c2 = null;

MartinNowak · 2015-05-06T07:46:59Z

Mmh, I guess we could add a null state for RefCounted.
Default construction has problems with a nullary class constructors though.

auto c = RefCounted!C(); // this is null
auto c2 = refCounted!C(); // this is not

JakobOvrum · 2015-05-06T08:05:24Z

Ouch. std::shared_ptr gets around this by delegating the onus of construction to the user, but this is not an option for us.

edit:

Construction of the supertype T, that is.

MartinNowak · 2015-05-06T10:33:52Z

This difference might be OK though, because one is clearly a function call while the other is a struct literal.

burner · 2015-05-11T09:06:50Z

std/typecons.d

+        assert(Foo.ctor == 1 && Foo.dtor == 0);
+    }
+    assert(Foo.ctor == 1 && Foo.dtor == 1);
+}


could add a test for the inheritance case

MartinNowak · 2015-05-12T23:24:43Z

Changed the implementation to account for the discussed changes.
There is still some stuff left to be done, I'd highly appreciate any help, e.g. polishing the implementation, improving tests, documentation, and examples or implementing nullable for Unique.

JakobOvrum · 2015-05-13T06:11:01Z

std/typecons.d

+    }
+
+    /// Boolean conversion for RefCounted, true when initialized.
+    bool opCast(T:bool)() const @safe


This notation catches things like cast(int) as well, it's probably better to use bool opCast(T)() if(is(T == bool))

No it does not Error: template instance opCast!int does not match template declaration opCast(T : bool)(), parameter specializations are exact matches despite the colon.

Huh, didn't know that, thanks.

It's weird that the syntax deviates from the is expression, just another accidental complexity.

MartinNowak · 2015-05-13T07:09:36Z

Left to be done.

implement proper destruction
polish documentation

mrkline · 2015-05-14T08:35:47Z

I am terribly sorry I didn't chime in sooner - I was on vacation the past two weeks.

A lot of what @JakobOvrum has said above really resonates with my. Frankly, if we're going to overhaul Unique with this upcoming release, RefCounted should get a similar makeover. The two go hand in hand. As we all discussed in the Unique PR, now is not the time for half-measures. Now is a good time to rip them up and Do It Right™. RefCounted should:

Have much in common with the new Unique, including
- get() functions
- an empty property and opCast(T:bool) indicating whether the underlying pointer is null
- alias get this
- A single refCounted function which takes the constructor arguments for the underlying type and returns a RefCounted
Get rid of all the lazy initialization stuff such as autoInit and ensureInitialized(). This is a can of worms a smart pointer type shouldn't open.
Get rid of the refCountedStore() function. The user shouldn't concern themselves with the underlying storage. That's an implementation detail the user should have no need to know, provided we provide correct reference counting semantics.

In short, its API should be nearly identical to that of Unique (possibly with the addition of a refCount property) , just like shared_ptr has so much in common with unique_ptr.

Again, I'm sorry for all the legwork you've already done, Martin - I'll help in any way I can now that I'm back, including taking up these changes in my own PR.

MartinNowak · 2015-05-14T13:45:08Z

A lot of what @JakobOvrum has said above really resonates with my. Frankly, if we're going to overhaul Unique with this upcoming release, RefCounted should get a similar makeover. The two go hand in hand. As we all discussed in the Unique PR, now is not the time for half-measures. Now is a good time to rip them up and Do It Right™. RefCounted should:

Not quite, RefCounted is widely used, so we cannot make radical changes.

get() functions
an empty property and opCast(T:bool) indicating whether the underlying pointer is null
alias get this

The implicit bool conversion is there, but get and empty could hide functions of the underlying type making things like a RefCounted range impossible. We should probably revise Unique to account for this.

A single refCounted function which takes the constructor arguments for the underlying type and returns a RefCounted

Will extend refCounted it to non-class types.

Get rid of all the lazy initialization stuff such as autoInit and ensureInitialized(). This is a can of worms a smart pointer type shouldn't open.

I agree, we'll have to see how to deprecate it properly.

Get rid of the refCountedStore() function. The user shouldn't concern themselves with the underlying storage. That's an implementation detail the user should have no need to know, provided we provide correct reference counting semantics.

I did that before, named it payload, but the purpose of refCountedStore is to namespace functions of RefCounted. Might make sense to drop this, b/c most RefCounted methods are unambiguous.

In short, its API should be nearly identical to that of Unique (possibly with the addition of a refCount property) , just like shared_ptr has so much in common with unique_ptr.

Right, let's see how far we can get.

Again, I'm sorry for all the legwork you've already done, Martin - I'll help in any way I can now that I'm back, including taking up these changes in my own PR.

You can make PRs that target my branch to collaborate on this.

- add a RefCounted variant for classes - remove the autoInit stuff which isn't applicable for classes - @nogc and nothrow compatible

MartinNowak · 2015-05-14T14:19:24Z

The implicit bool conversion is there, but get and empty could hide functions of the underlying type

I think get and refCount should be fine, empty isn't needed as opCast(T:bool)() can be used instead.

mrkline · 2015-05-14T15:39:06Z

Ah, I didn't realize that RefCounted was more widely used, so perhaps we need to move a bit slower. I also forgot Github lets you target PRs at arbitrary branches. I'll get poking around. Thanks again for the help.

MartinNowak · 2015-05-14T16:14:04Z

I added support for any type in refCounted.

Ah, I didn't realize that RefCounted was more widely used, so perhaps we need to move a bit more slowly. I also forgot Github lets you target PRs at arbitrary branches. I'll get poking around. Thanks again for the help.

Meanwhile I'll work on the destruction problem.

JakobOvrum · 2015-05-14T17:24:38Z

The implicit bool conversion is there, but get and empty could hide functions of the underlying type making things like a RefCounted range impossible. We should probably revise Unique to account for this.

I have proposed to remove Unique.empty in #3225.

mrkline · 2015-05-17T04:10:17Z

I'll hit the ground with this once after we settle on what API we want in #3225.

MartinNowak · 2015-05-17T10:33:44Z

Meanwhile I'll work on the destruction problem.

Done, dlang/dmd#4650.

HK47196 · 2015-06-11T06:31:53Z

ping, what's the status of this?

mrkline · 2015-06-11T20:05:48Z

I believe we're waiting for #3225 to shake out before deciding what to do here.

MartinNowak · 2015-07-01T09:19:55Z

TODO:

Use isNull and nullify API
use __xdtor to recursively destruct classes

MartinNowak · 2015-07-23T06:09:23Z

Closing in favor of a complete PR of all smart refs (WIP https://github.com/MartinNowak/phobos/tree/smartRefs).

MartinNowak force-pushed the master branch from d4bf71e to 38f13c2 Compare May 6, 2015 06:58

burner reviewed May 11, 2015
View reviewed changes

MartinNowak force-pushed the master branch 2 times, most recently from f5ebe0c to cf69dfd Compare May 12, 2015 23:22

MartinNowak force-pushed the master branch from cf69dfd to 73775ad Compare May 12, 2015 23:40

JakobOvrum reviewed May 13, 2015
View reviewed changes

MartinNowak force-pushed the master branch from 73775ad to 8a410f7 Compare May 13, 2015 07:05

MartinNowak force-pushed the master branch from 8a410f7 to 24e1547 Compare May 13, 2015 07:12

MartinNowak added 2 commits May 14, 2015 15:54

cleanup whitespace

dec1d04

RefCounted implementation for classes

8cf0ec2

- add a RefCounted variant for classes - remove the autoInit stuff which isn't applicable for classes - @nogc and nothrow compatible

MartinNowak force-pushed the master branch from 24e1547 to 8cf0ec2 Compare May 14, 2015 14:17

mrkline mentioned this pull request Jun 1, 2015

Further changes to std.typecons.Unique #3225

Closed

MartinNowak added this to the 2.068 milestone Jun 30, 2015

MartinNowak closed this Jul 23, 2015

MartinNowak removed this from the 2.068 milestone Jul 23, 2015

Uh oh!

RefCounted implementation for classes #3259

RefCounted implementation for classes #3259

Uh oh!

Conversation

MartinNowak commented May 6, 2015

Uh oh!

MartinNowak commented May 6, 2015

Uh oh!

HK47196 commented May 6, 2015

Uh oh!

JakobOvrum commented May 6, 2015

Uh oh!

MartinNowak commented May 6, 2015

Uh oh!

MartinNowak commented May 6, 2015

Uh oh!

JakobOvrum commented May 6, 2015

Uh oh!

MartinNowak commented May 6, 2015

Uh oh!

JakobOvrum commented May 6, 2015

Uh oh!

MartinNowak commented May 6, 2015

Uh oh!

burner May 11, 2015

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented May 12, 2015

Uh oh!

JakobOvrum May 13, 2015

Choose a reason for hiding this comment

Uh oh!

MartinNowak May 13, 2015

Choose a reason for hiding this comment

Uh oh!

JakobOvrum May 13, 2015

Choose a reason for hiding this comment

Uh oh!

MartinNowak May 13, 2015

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented May 13, 2015

Uh oh!

mrkline commented May 14, 2015

Uh oh!

MartinNowak commented May 14, 2015

Uh oh!

MartinNowak commented May 14, 2015

Uh oh!

mrkline commented May 14, 2015

Uh oh!

MartinNowak commented May 14, 2015

Uh oh!

JakobOvrum commented May 14, 2015

Uh oh!

mrkline commented May 17, 2015

Uh oh!

MartinNowak commented May 17, 2015

Uh oh!

HK47196 commented Jun 11, 2015

Uh oh!

mrkline commented Jun 11, 2015

Uh oh!

MartinNowak commented Jul 1, 2015

Uh oh!

MartinNowak commented Jul 23, 2015

Uh oh!

Uh oh!