Skip to content

Unseal containers #1857

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

Merged
merged 4 commits into from
Feb 15, 2014
Merged

Unseal containers #1857

merged 4 commits into from
Feb 15, 2014

Conversation

monarchdodra
Copy link
Collaborator

See #1845 & https://d.puremagic.com/issues/show_bug.cgi?id=11889 .

Per @andralex and walter (http://forum.dlang.org/thread/mailman.281.1389340467.15871.digitalmars-d@puremagic.com#post-lap6dn:2483a:241:40digitalmars.com), this "unseals" containers.

@andralex , I think this is a more "correct" version of what you threw together in #1845 . I'm assigning it to you.


I ended up keeping the move family of functions because:
a) Outright removing them would require the caller to have std.range imported to not break code.
b) They can't be deprecated, as the deprecated function would have priority over the ufcs range overloads.
c) The range overloads would only work for the ranges, but not the containers themselves.
So I ended up doing nothing about it. It can't hurt anyone. Also, funny story, DList doesn't have them.

@andralex
Copy link
Member

Oops, this duplicates a bunch of the work that I had in progress... but no matter. I'll review this.

@monarchdodra
Copy link
Collaborator Author

Oops, this duplicates a bunch of the work that I had in progress...

:( I was hoping you hadn't started any work yet. Sorry. Well, if you were almost done, you can submit your code and I can review too. I just wanted to see this go through, and I wasn't sure you were going to submit anything...

But no matter. I'll review this.

Thanks.

@@ -306,7 +306,7 @@ Generally a container may define several types of ranges.
assert(0);
}
/// Ditto
@property T back()
@property ref T back()
Copy link
Member

Choose a reason for hiding this comment

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

This is for developer documentation, so it should mention that ref is optional.

@andralex
Copy link
Member

lgtm modulo comments

@monarchdodra
Copy link
Collaborator Author

Comment addressed. I think. Is this what you had in mind?

I also corrected the docs a bit: Some functions were missing, and others were outright wrong (arguments were incorrect).

@jcd
Copy link
Contributor

jcd commented Jan 17, 2014

LGTM as well. Got bitten by this issue myself just today.

@monarchdodra
Copy link
Collaborator Author

Ping

@DmitryOlshansky
Copy link
Member

poke @andralex

@monarchdodra
Copy link
Collaborator Author

Pokity-ping @andralex .

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

@DmitryOlshansky
Copy link
Member

With Andrei being remarkably busy, I'm going on limb and auto-merging this.

DmitryOlshansky added a commit that referenced this pull request Feb 15, 2014
@DmitryOlshansky DmitryOlshansky merged commit 969d7fe into dlang:master Feb 15, 2014
@monarchdodra
Copy link
Collaborator Author

With Andrei being remarkably busy

Yeah, too bad about that :/

I'm going on limb and auto-merging this.

Thanks :)

@monarchdodra monarchdodra deleted the unseal branch February 18, 2014 19:36
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.

4 participants