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

Allow other int types for type container size #893

Conversation

KronosTheLate
Copy link
Contributor

This PR aims to solve #891, and check if this issue occurs for other types as well. For the types listed in the documentation, the results are seen in the drop-down below.

Deque (implemented with an unrolled linked list) errors on `Deque{Float64}(Int32(10))`. Fixed

CircularBuffer
Silent error on CircularBuffer{Float32}(Int32(10)). Fixed

CircularDeque (based on a circular buffer)
errors on CircularDeque{Float32}(Int32(10)). Fixed

Stack
errors on Stack{Float64}(Int32(5)). Got fixed, I think with Deque

Queue
errors on Queue{Float64}(Int32(5)). Got fixed, I think with Deque

Priority Queue
No length parameter upon initialization

Fenwick Tree
Works for FenwickTree{Float64}(Int32(5)) and FenwickTree{Float64}(Int16(5))

No length parameter upon initialization for the following types
Accumulators and Counters (i.e. Multisets / Bags)
Disjoint Sets
Binary Heap
Mutable Binary Heap
Ordered Dicts and Sets
RobinDict and OrderedRobinDict (implemented with Robin Hood Hashing)
SwissDict (inspired from SwissTables)
Dictionaries with Defaults
Trie
Linked List and Mutable Linked List
Sorted Dict, Sorted Multi-Dict and Sorted Set
DataStructures.IntSet
SparseIntSet
DiBitVector
Red Black Tree
AVL Tree
Splay Tree

Comment on lines 14 to +15
CircularDeque{T}(n::Int) where {T} = CircularDeque(Vector{T}(undef, n), n, 0, 1, n)
CircularDeque{T}(n::Integer) where {T} = CircularDeque(Vector{T}(undef, Int(n)), Int(n), 0, 1, Int(n))
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just do

Suggested change
CircularDeque{T}(n::Int) where {T} = CircularDeque(Vector{T}(undef, n), n, 0, 1, n)
CircularDeque{T}(n::Integer) where {T} = CircularDeque(Vector{T}(undef, Int(n)), Int(n), 0, 1, Int(n))
CircularDeque{T}(n::Integer) where {T} = CircularDeque(Vector{T}(undef, n), n, 0, 1, n)

since constructors automatically call convert on all arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though about that option, but it somehow seemed more clear to me to explicitly do what will happen automatically. It somehow makes the code more readable to me, because it shows it's purpose more clearly. But I do not have strong opinions, so if you prefer this, it is fine by me ^_^

Copy link
Member

Choose a reason for hiding this comment

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

I see your point (getting rid of this behavour has actually been talked about for julia 2.0).

On the other hand, code that doesn't exist can't have bugs.

Comment on lines +22 to +24

# Convert any `Integer` to whatever `Int` is on the relevant machine
CircularBuffer{T}(f::Integer, len::Integer, buf::Integer) where {T} = CircularBuffer{T}(Int(f), Int(len), Int(buf))
Copy link
Member

Choose a reason for hiding this comment

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

Per above i don't think this is needed since constructors automatically convert.
Even if you have another inner constructor.

Suggested change
# Convert any `Integer` to whatever `Int` is on the relevant machine
CircularBuffer{T}(f::Integer, len::Integer, buf::Integer) where {T} = CircularBuffer{T}(Int(f), Int(len), Int(buf))

Comment on lines +23 to +25

# Convert any `Integer` to whatever `Int` is on the relevant machine
DequeBlock{T}(capa::Integer, front::Integer) where T = DequeBlock{T}(Int(capa), Int(front))
Copy link
Member

Choose a reason for hiding this comment

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

Per above

Suggested change
# Convert any `Integer` to whatever `Int` is on the relevant machine
DequeBlock{T}(capa::Integer, front::Integer) where T = DequeBlock{T}(Int(capa), Int(front))

@KronosTheLate
Copy link
Contributor Author

Regarding your reviews, I though you were mainly changing my ecplixit conversion via Int(arg). But I had to add the lines in the first place to make it work. Have you tested your suggestions?

@oxinabox
Copy link
Member

Have you tested your suggestions?

I have not.
If you say they proved nesc then I will go with that.
As my opinon is not strong

@oxinabox oxinabox merged commit 45386fb into JuliaCollections:master Jan 30, 2024
8 of 10 checks passed
@oxinabox
Copy link
Member

Do you need this backported to 0.18?

@timholy
Copy link
Member

timholy commented Jan 30, 2024

Fun fact: sometimes it's useful to use the Foo(n::Integer, args...) = Foo(Int(n), args...) pattern even for constructors that perform automatic conversion. The reason? It confines specialization-diversity to a short "stub" function, and the "long function" (the one that does the actual work) gets compiled for a narrow range of types. I.e., less latency.

That said, here the constructors are short so I don't think it's a big deal either way.

@KronosTheLate
Copy link
Contributor Author

Do you need this backported to 0.18?

If you are asking me, then no ^_^

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