Skip to content

Fixes & docs for DList #910

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 3 commits into from
Nov 1, 2012
Merged

Fixes & docs for DList #910

merged 3 commits into from
Nov 1, 2012

Conversation

monarchdodra
Copy link
Collaborator

Addresses all issues in:
http://forum.dlang.org/thread/gjhclwsuqyhrimdeoaec@forum.dlang.org
and
http://d.puremagic.com/issues/show_bug.cgi?id=8905

  1. Adds extra documentation on what to expect from range operations.
  2. Fixes a lot of corner cases, when trying to push after poping, inparticular, the two bugs in 8905.
  3. Renamed opOpassign into opOpAssign
  4. Fixed the "~" operators, which were not behaving according to docuementation (and tested them in unit tests)
  5. Major re-implemention of remove and insertBeforeNode, which were more or less responsible for all the bugs in DList.

Also, tons of unittests.

Finally: And this is kind of major, I scheduled for deprecation "stableRemove(R)" and "stableLinearRemove(R)", since neither could actually guarantee stability. For example:

auto a = DList([1, 2, 3]);
auto r1 = a[];
auto r2 = a[];
r1.popBack(); //[1, 2]
r2.popFront(); //[2, 3]
a.stableRemove(r1);/// DERP!
//r2 is now invalid.

@alexrp
Copy link
Member

alexrp commented Oct 30, 2012

  • f8f0bee: OK, but could use a better commit message.
  • 8ccea19: OK.
  • 09d4d94: OK, but slightly misleading commit message.
  • 5e79b94: OK.
  • 73b9c49: OK.

(I recommend squashing all those white space changes into one commit if possible.)

  • e8b9c0c: OK.

@monarchdodra
Copy link
Collaborator Author

Thanks you for the re-read. Yes, I'll squash, but I can't do it from this current computer. I'll also reword the commit message.

@alexrp
Copy link
Member

alexrp commented Oct 31, 2012

Anyone else want to have a look at this?

@monarchdodra
Copy link
Collaborator Author

6c38c80 improves linearRemove in the sense that it works only on take ranges, and not all the ranges that have a source attribute:

Ranges such as retro, stride and others also have source, but accepting them as input would not make sense.

Also added documentation about what to expect from linearRemove (which was just documented as ///ditto)

EDIT: No new tests for linearRemove, as it was already covered.

@alexrp
Copy link
Member

alexrp commented Nov 1, 2012

Any other pending changes or is this good to merge?

Also, slightly related since you're poking around in DList anyway: How possible do you think it would be to use manual memory management (malloc/emplace/free) for nodes rather than the GC?

@monarchdodra
Copy link
Collaborator Author

Any other pending changes or is this good to merge?

I don't think I have anything else to add on my end, pertaining to this change.

Also, slightly related since you're poking around in DList anyway: How possible do you think it would be to use manual memory management (malloc/emplace/free) for nodes rather than the GC?

Quick answer:
We could reference count entire "chain at once", but then the gains would be minimal.
We could try to destroy nodes individually as soon as they became un-referenced. That would definitly require more bookkeeping though, and certain operations which were 0(1) might become 0(N).

@alexrp
Copy link
Member

alexrp commented Nov 1, 2012

Okay, LGTM.

It's a bit annoying that we can't sanely avoid the GC for DList - I'm a bit afraid that the cost of using the GC negates any gain of using it over a plain array list for small workloads. But I should probably benchmark it...

alexrp pushed a commit that referenced this pull request Nov 1, 2012
@alexrp alexrp merged commit ded7d06 into dlang:master Nov 1, 2012
@monarchdodra
Copy link
Collaborator Author

It's a bit annoying that we can't sanely avoid the GC for...

This is all assuming we keep the "chain" mechanic we have going on here. It is the way it currently works, but I'm not even sure it was intended to work this way... I think the original author just didn't really think through what would happen if several DLists aliased the same content...

I merely patched a few holes I found because of this. The current mechanic is un-natural: DList is both container and range (range in the sense that it offers a view into something).

IMO, DList should be changed to reference semantic, in that each DList will point to the same shared payload (a pointer pair [_first, _last]), and that all those DLists have the exact same view of that payload.

If we do this, not only will DList work exactly the way people expect it to, we'll also gain in terms of implementation complexity.

I wouldn't mind implementing it, but given I'd change the fundamental nature of the object, I'd want confirmation. If we do do this, then all the doc I just added would be wrong though (and some of the fixes here unneeded) :/

Should I go this direction?

@alexrp
Copy link
Member

alexrp commented Nov 1, 2012

Perhaps we could have two doubly-linked lists; one being the current implementation and the other being what you just described. Thoughts? (Ideas for names?)

@denis-sh
Copy link
Contributor

denis-sh commented Nov 2, 2012

@monarchdodra, please, separate e.g. docs changes and refactoring changes from logical. And, for such large diffs, also segregating addition of unittests may be useful.

To do this, you may want to use SmartGit as it allows easy editing index before commit and very good diff view (but it have almost no rebasing support so I use SmartGit for this 2 things only).

@monarchdodra
Copy link
Collaborator Author

Perhaps we could have two doubly-linked lists; one being the current implementation and the other being what you just described. Thoughts? (Ideas for names?)

If you are still reading, I have a preliminary proposal here:

http://forum.dlang.org/thread/zjjcupycjnqzoqwxrhyz@forum.dlang.org

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.

3 participants