-
Notifications
You must be signed in to change notification settings - Fork 246
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
Allow other int types for type container size #893
Conversation
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)) |
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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 ^_^
There was a problem hiding this comment.
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.
|
||
# 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)) |
There was a problem hiding this comment.
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.
# 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)) |
|
||
# 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above
# 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)) |
Regarding your reviews, I though you were mainly changing my ecplixit conversion via |
I have not. |
Do you need this backported to 0.18? |
Fun fact: sometimes it's useful to use the That said, here the constructors are short so I don't think it's a big deal either way. |
If you are asking me, then no ^_^ |
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.
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