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

Isue 4424 & 6216 Relax opAssign signature #166

Merged
merged 5 commits into from Oct 7, 2012

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Jun 26, 2011

This change is based on #94 (already merged).

Issue 4424 - Copy constructor and templated opAssign cannot coexist
Issue 6174 - Initialize const fixed-size array in constructor
Issue 6216 - Built-in opAssign implicitly defined should call field's opAssign
Issue 6336 - Can't return ref T where T has const/immutable members

Following is removed from this pull request:
Issue 4596 - [tdpl] Rebinding this in class method compiles

@cristicbz
Copy link

I would be really interested in this request going through as Issue 4424 seriously affects the interface of my GSoC project.

@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 17, 2011

I would be really interested in this request going through as Issue 4424 seriously affects the interface of my GSoC project.

A workaround exists against Issue 4424 like follows:

struct X {
    private mixin template _workaround4424()
    {
        @disable void opAssign(typeof(this) );
    }
    mixin _workaround4424;

    void opAssign(R)(R rhs) { /*...*/ }
}

It is already used std.typecons.Tuple.

braddr pushed a commit to braddr/dmd that referenced this pull request Sep 15, 2011
@9rnsr 9rnsr closed this Nov 4, 2011
@9rnsr 9rnsr reopened this Nov 6, 2011
@9rnsr
Copy link
Contributor Author

9rnsr commented Nov 6, 2011

Update patch.
This additionaly fix issue 6174.

@9rnsr
Copy link
Contributor Author

9rnsr commented Dec 25, 2011

Updated to fix 4596.

@repeatedly
Copy link
Member

I hit this issue in msgpack-d today.
I removed 'immutable' keyword from member variables for avoiding compilation error.

This patch is needed for correct implementation.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jan 24, 2012

This pull also contains a fix for bug 6336.
Sorry, I had completely forgotten to post/find an issue about that problem in bugzilla.

@yebblies
Copy link
Member

yebblies commented Oct 6, 2012

Would it be possible to break this up a bit to make it easier to review and merge?

@dnadlinger
Copy link
Member

@yebblies: As far as I can see, the pull request is already broken up into atomic commits?!

@nazriel
Copy link
Contributor

nazriel commented Oct 6, 2012

After excluding unittests this pull isn't THAT big, but still I think @yebblies is right here.
1 bug - 1 pull request would be easier to review for folks even if Pull request is divided into atomic commits.

@yebblies
Copy link
Member

yebblies commented Oct 6, 2012

@klickverbot It's easier if I can review once bugfix at a time. Is each commit completely independent of the others? If so, it will be fairly easy to split them into multiple pull requests. If not I'll need to work out how they are related before I can be confident enough to pull them.

Class type that has identity opAssign is disallowed both it is templated or not.
…ld's opAssign

Separate 'top assignable' (see opAssign first) from 'blit assignable' (memberwise), and now they are not related to 'modifiable' directly.
Remove fail7695.d, because it was incorrectly failed to compile by bug 6336.
Improve checking of whether an expression is modifiable.
If an expression is *initializing*, that is part of construction, then it bypass type check.
@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 7, 2012

Later commits depends on early ones, and all of them are related to the assignable language semantics.
OK. The commits for 6174 and 4596 are additional changes, so we don't need merge them at once. I'll remove the additionals from this pull request.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 7, 2012

Updated.

@yebblies
Copy link
Member

yebblies commented Oct 7, 2012

This seems to be causing some failures in std.container

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 7, 2012

The error in std.container is fixed by the change for 6174...
Unfortunately, I can not break up the changes, except for 4596.

@yebblies
Copy link
Member

yebblies commented Oct 7, 2012

Fair enough.

yebblies added a commit that referenced this pull request Oct 7, 2012
Isue 4424 & 6216 Relax opAssign signature
@yebblies yebblies merged commit 5b42e51 into dlang:master Oct 7, 2012
@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 7, 2012

I opened #1169 for issue 4596.


Lassignerr:
if (fd && !(fd->storage_class & STCdisable))
error("identity assignment operator overload is illegal");
Copy link
Member

Choose a reason for hiding this comment

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

Why would you allow to explicitly @disable the operator if it is illegal anyhow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because @disable opAssign can disallow class reference rebinding. If we allow normal opAssign for class types, it will cause null dereference if assigned class reference is null. On the other hand, disabled opAssign is always checked in compile time and no runtime error happens. I thought it would be useful.

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