Skip to content

Add MutableLinkedList #450

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 14 commits into from
Feb 26, 2019
Merged

Add MutableLinkedList #450

merged 14 commits into from
Feb 26, 2019

Conversation

c-p-murphy
Copy link
Contributor

I put this together in response to issue #443. This is my first real contribution julia, so feel free to share any advice and/or feedback about how I could improve this implementation.

@codecov
Copy link

codecov bot commented Sep 23, 2018

Codecov Report

Merging #450 into master will decrease coverage by 0.59%.
The diff coverage is 81.45%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #450     +/-   ##
=========================================
- Coverage   91.32%   90.73%   -0.6%     
=========================================
  Files          29       30      +1     
  Lines        2133     2255    +122     
=========================================
+ Hits         1948     2046     +98     
- Misses        185      209     +24
Impacted Files Coverage Δ
src/DataStructures.jl 100% <ø> (ø) ⬆️
src/mutable_list.jl 81.45% <81.45%> (ø)
src/heaps/arrays_as_heaps.jl 91.17% <0%> (-2.95%) ⬇️
src/deque.jl 97.84% <0%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b11a10f...63fed45. Read the comment docs.

@c-p-murphy c-p-murphy closed this Sep 23, 2018
@c-p-murphy c-p-murphy reopened this Sep 23, 2018
@oxinabox
Copy link
Member

Thanks for doing this.

@ararslan, @kmsquire I am really busy right now.
could either of you take a look at this?

Cross-ref
#442 and #443

Generally, speaking I am kinda opposed to adding more DataStructures to Data Structures.jl.
as I still think we really need to be doing: #310

But with that said, #310 is a fair bit of work and I've failed at it once, and don't see myself having free time to run with it anytime soon.
As such it really shouldn't block other things going forward, I guess.

Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

I think this is a good home for a mutable linked list type until the package is split up as Lyndon noted, if that ever fully happens. I just have a few comments.

@ararslan
Copy link
Member

ararslan commented Sep 24, 2018

This is looking good to me, though it'd be nice to have another set of eyes on it as well.

Copy link
Member

@kmsquire kmsquire left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. I think it would be nice to bring the other LinkedList up-to-date and make the interface look more like this one. But that doesn't have to happen with this PR.

@kmsquire
Copy link
Member

kmsquire commented Feb 19, 2019 via email

@c-p-murphy
Copy link
Contributor Author

c-p-murphy commented Feb 20, 2019

Thanks 😄

Unfortunately, it looks like there is (I forgot to check). @code_warntype highlights these lines red:

julia> @code_warntype map(f, l)
Body::MutableLinkedList{_1} where _1
.
.
.
5 ── . . .
│    %9  = (Core.Compiler.return_type)(f, %8)::Type%10 = (Core.apply_type)(DataStructures.MutableLinkedList, %9)::Type{MutableLinkedList{_1}} where _1
│    %11 = (%10)()::MutableLinkedList{_1} where _1
.
.
.

Not sure what the best way to fix it is though. Is there a recommended approach?

@kmsquire
Copy link
Member

Not that I'm aware of. In this case I think we just live with it.

Thanks for the contribution!

@kmsquire kmsquire merged commit 66a0a2d into JuliaCollections:master Feb 26, 2019
@c-p-murphy c-p-murphy deleted the mutable_list branch March 5, 2019 02:25
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