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

DList cleanup #1954

Merged
merged 1 commit into from Feb 24, 2014
Merged

DList cleanup #1954

merged 1 commit into from Feb 24, 2014

Conversation

monarchdodra
Copy link
Collaborator

This is a preliminary edit to cleanup DList. It contains only mostly trivial edits I want to get out of the way.

Changes include:

  • Relaxing certain template restrictions thanks to smarter compiler overload resolution. Also, un-template certain functions (this linearRemove(Take!Range r)), because we can now overload templates and non-templates
  • Templatize and constraint opEquals, for types that aren't actually comparable. I ignored the issue of mutable comparison.
  • Make front/back inout.
  • Add more checks in Range for invalidated ranges.
  • A lot of "opConcat" re-write. Not sure it is worth looking at the diff on that one. Speaking of which, I question being able to concat a DList we have ranges and the slice operator.

Lastly, I moved around some functions, and marked sections, to have a clearer idea of the class layout: Concat, Insertion, Removal. Because of this, there is some noise in the diff, but I can guarantee NO CHANGES were made to code that was moved.

PS: I'll squash when the pull is approved.

@monarchdodra
Copy link
Collaborator Author

concat section now looks like this:

/**
Returns a new $(D DList) that's the concatenation of $(D this) and its
argument $(D rhs).
     */
    DList opBinary(string op, Stuff)(Stuff rhs)
    if (op == "~" && is(typeof(insertBack(rhs))))
    {
        auto ret = this.dup;
        ret.insertBack(rhs);
        return ret;
    }
    /// ditto
    DList opBinary(string op)(DList rhs)
    if (op == "~")
    {
        return ret ~ rhs[];
    }

/**
Returns a new $(D DList) that's the concatenation of the argument $(D lhs)
and $(D this).
     */
    DList opBinaryRight(string op, Stuff)(Stuff lhs)
    if (op == "~" && is(typeof(insertFront(lhs))))
    {
        auto ret = this.dup;
        ret.insertFront(lhs);
        return ret;
    }

/**
Appends the contents of the argument $(D rhs) into $(D this).
     */
    DList opOpAssign(string op, Stuff)(Stuff rhs)
    if (op == "~" && is(typeof(insertBack(rhs))))
    {
        insertBack(rhs);
        return this;
    }

/// ditto
    DList opOpAssign(string op)(DList rhs)
    if (op == "~")
    {
        return this ~= rhs[];
    }

@DmitryOlshansky
Copy link
Member

I think you'd better squash 'em anyway - the latter 2 commits are minor edits of the first one.

@monarchdodra
Copy link
Collaborator Author

I think you'd better squash 'em anyway - the latter 2 commits are minor edits of the first one.

Oops. Yeah, I meant to do that, but forgot. Done.

auto a1 = IntList(0);
auto a2 = IntList(0, 1);
auto a3 = IntList([0]);
auto a4 = IntList([0, 1]);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe some kind of a4[].equal([0, 1])tests are warranted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@DmitryOlshansky
Copy link
Member

Otherwise LGTM

DmitryOlshansky added a commit that referenced this pull request Feb 24, 2014
@DmitryOlshansky DmitryOlshansky merged commit 10f75ad into dlang:master Feb 24, 2014
@monarchdodra
Copy link
Collaborator Author

Thanks :)
Now, time to do some real changes.

@9rnsr
Copy link
Contributor

9rnsr commented Aug 11, 2014

This change introduces regression https://issues.dlang.org/show_bug.cgi?id=13279

But, I think it seems expected change by this PR.
@monarchdodra Could you please see the bugzilla issue?

@monarchdodra
Copy link
Collaborator Author

@monarchdodra Could you please see the bugzilla issue?

Thanks for the heads up. Replied there.

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