Skip to content
New issue

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

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

fix Issue 14432 - move construction for RefCounted #3171

Merged
merged 3 commits into from
Apr 20, 2015

Conversation

MartinNowak
Copy link
Member

  • add RefCounted!T.this(T) which takes an RValue or a copy
    and use move to initialized the refcounted store
  • add refCounted to infer T from the argument and disable
    RefCounted's autoInit, also move initializes the store

import core.stdc.string : memcpy;
import std.exception : enforce;

_store = cast(Impl*) enforce(malloc(Impl.sizeof));
Copy link
Member

Choose a reason for hiding this comment

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

You can't use enforce for malloc, use core.exception.onOutOfMemoryError.

See also #3031.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

- add RefCounted!T.this(T) which takes an RValue or a copy
  and use move to initialized the refcounted store

- add refCounted to infer T from the argument and disable
  RefCounted's autoInit, also move initializes the store
{
import std.algorithm.searching : endsWith;

static T empty;
Copy link
Member

Choose a reason for hiding this comment

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

Why put this in TLS instead of the stack? Could also use typeid(T).init() like destroy does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmh, I just copied the move code. Updated.

@MartinNowak
Copy link
Member Author

Also changed move to use typeid(T).init() instead of an "empty" TLS variable.
This stuff would be way move efficient, if we had destructive move.

@MartinNowak
Copy link
Member Author

Once #3139 is ready, we should specifically handle promoting a Unique!T to a RefCounted!T.

@JakobOvrum
Copy link
Member

LGTM

@MartinNowak
Copy link
Member Author

Well then go ahead merge it ;).

@JakobOvrum
Copy link
Member

Ideally a third person would look over it, but it's already been 10 days so...

@JakobOvrum
Copy link
Member

Auto-merge toggled on

JakobOvrum added a commit that referenced this pull request Apr 20, 2015
fix Issue 14432 - move construction for RefCounted
@JakobOvrum JakobOvrum merged commit f7498ad into dlang:master Apr 20, 2015
@MartinNowak MartinNowak deleted the refCounted branch April 20, 2015 14:38
@MartinNowak
Copy link
Member Author

Ideally a third person would look over it

Ideally we'd handle PR within a few days.
I made a best-effort implementation, you did a best-effort review, there is nothing wrong with making mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants