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

Make move CTFEable for simple structs and classes #2036

Merged
merged 1 commit into from Mar 28, 2014

Conversation

DmitryOlshansky
Copy link
Member

memcpy is not CTFEable, so avoid it for structs without
dtor, postblit that also can be assigned with plain lhs = rhs;.

This leaves as is (compile error in CTFE) structs that contain
immutable/const memebers the fact that moves overwrites such
fields is questionable to begin with.

Remark: pointsTo doesn't work at CTFE. The check itself makes no sense for class instances and primitive types.

// Most complicated case. Destroy whatever target had in it
// and bitblast source over it
static if (hasElaborateDestructor!T) typeid(T).destroy(&target);

memcpy(&target, &source, T.sizeof);
static if (hasElaborateCopyConstructor!T || hasElaborateDestructor!T
|| !isAssignable!T)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You also need to test hasElaborateAssign.

Once you test hasElaborateAssign, I don't think there's any need to test for hasElaborateCopyConstructor!T or hasElaborateDestructor!T either. Just:

static if (hasElaborateAssign!T || !isAssignable!T)

In that order. Unless I missed something? It's what we do in the rest of phobos anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems you are right except for hasElaborateDestructor, since we must do memcpy to avoid calling destructor on void-initialized result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are over thinking it. If anything cause a = * to do anything other than a straight up bit copy, then a will have an elaborate assign, and be marked as such, and that will be the end of that.

struct S
{
    ~this(){}
}

void main() 
{
    static assert(hasElaborateAssign!S);
}

So I agree that "destructor" => "must do memcpy", but you are already covered by "destructor" => "elaborate assign".

@DmitryOlshansky
Copy link
Member Author

@monarchdodra
As noted destructor must be checked separately.
As to isElaborateAssign vs ElaborateCopyConstructor - I'd rather keep things the same way they were originally.

@monarchdodra
Copy link
Collaborator

As noted destructor must be checked separately.

Yes, there are two separate operations, with two separate tests:

  • knowing if you can call assign: !hasElaborateAssign!T && isAssignable!T
  • dealing with destruction: hasElaborateCopyConstructor!T || hasElaborateDestructor!T

As to isElaborateAssign vs ElaborateCopyConstructor - I'd rather keep things the same way they were originally.

Dealing with destruction is indeed unchanged. The assignement test is new, so there's no "keeping things the same way they were originally".


In any case, hasElaborateAssign must indeed be checked for. Let's get it fixed and get this show on the road.

@DmitryOlshansky
Copy link
Member Author

In any case, hasElaborateAssign must indeed be checked for. Let's get it fixed and get this show on the road.

Well, I honestly do not care which combination of traits to use. hasElaborateAssign docs may need update to indicate compiler's generated ones count. Anyway changed to hasElaborateAssign.

@monarchdodra
Copy link
Collaborator

LGTM, but needs a rebase now.

hasElaborateAssign docs may need update to indicate compiler's generated ones count.

You or me? I tend to be not so good at writing documentation.

memcpy is not CTFEable, so avoid it for structs without
dtor, postblit that can be assigned with plain =.

This leaves as is (compile error in CTFE) structs that contain
immutable/const memebers the fact that moves overwrites such
fields is questionable to begin with.
@DmitryOlshansky
Copy link
Member Author

LGTM, but needs a rebase now.

Done.

You or me? I tend to be not so good at writing documentation.

I'll take a shot at docs.

@monarchdodra
Copy link
Collaborator

Auto-merge toggled on

monarchdodra added a commit that referenced this pull request Mar 28, 2014
Make move CTFEable for simple structs and classes
@monarchdodra monarchdodra merged commit 1863cb4 into dlang:master Mar 28, 2014
@DmitryOlshansky
Copy link
Member Author

Thanks!

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