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

Specify buffer length as Integer of any size #891

Closed
KronosTheLate opened this issue Jan 21, 2024 · 5 comments
Closed

Specify buffer length as Integer of any size #891

KronosTheLate opened this issue Jan 21, 2024 · 5 comments

Comments

@KronosTheLate
Copy link
Contributor

This is easiest to show in code. I will make a fresh environment and try to initialize a circular buffer:

(@v1.10) pkg> activate --temp
  Activating new project at `/tmp/jl_7Wbc1W`

(jl_7Wbc1W) pkg> add DataStructures
    Updating registry at `~/.julia/registries/General.toml`
   Resolving package versions...
    Updating `/tmp/jl_7Wbc1W/Project.toml`
⌃ [864edb3b] + DataStructures v0.18.15
    Updating `/tmp/jl_7Wbc1W/Manifest.toml`
⌃ [34da2185] + Compat v4.10.1
⌃ [864edb3b] + DataStructures v0.18.15
  [bac558e1] + OrderedCollections v1.6.3
  [2a0f44e3] + Base64
  [b77e0a4c] + InteractiveUtils
  [d6f4376e] + Markdown
  [9a3f8284] + Random
  [ea8e919c] + SHA v0.7.0
  [cf7118a7] + UUIDs
        Info Packages marked with ⌃ have new versions available and may be upgradable.

julia> using DataStructures

But using Int32 or Int16 errors:

julia> CircularBuffer{Float32}(Int32(10))
ERROR: MethodError: no method matching CircularBuffer{Float32}(::Int32)

Closest candidates are:
  CircularBuffer{T}(::Int64) where T
   @ DataStructures ~/.julia/packages/DataStructures/MKv4P/src/circular_buffer.jl:16

Stacktrace:
 [1] top-level scope
   @ REPL[10]:1

julia> CircularBuffer{Float32}(Int16(10))
ERROR: MethodError: no method matching CircularBuffer{Float32}(::Int16)

Closest candidates are:
  CircularBuffer{T}(::Int64) where T
   @ DataStructures ~/.julia/packages/DataStructures/MKv4P/src/circular_buffer.jl:16

Stacktrace:
 [1] top-level scope
   @ REPL[11]:1

Int64 works as expected:

julia> CircularBuffer{Float32}(Int64(10))
0-element CircularBuffer{Float32}

This could easily be fixed, by I wanted to ask some questions before making a PR:

  • Do we agree that this is unwanted behaviour?
  • Is it possible that this should be fixed for a class of structs defined in this package, or is it likely to only affect CircularBuffers?
@oxinabox
Copy link
Member

yes we want this.
because Vector{Float64}(undef, Int16(4)) etc works.

and yes we want to fix this for all types in the package.

It is fine if under the hood this just converts to Int (which is Int32 or Int64 depending on system).
Since that represents memory limits for that system

@KronosTheLate
Copy link
Contributor Author

Right. I know nothing about the types that exist for this package, and I am quite busy. Do you think someone will be around to fix this properly in the near future, or is it better if I give it an imperfect shot now? Fix something now is a lot better than fix everything never.

@oxinabox
Copy link
Member

Fix something now is a lot better than fix everything never.

If that is how you feel then yes it would be self consistent to open a PR to fix this now imperfectly

@KronosTheLate
Copy link
Contributor Author

I will give it ago. I am not doing what I am supposed to anyways today.

@KronosTheLate
Copy link
Contributor Author

Closed by #893

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

No branches or pull requests

2 participants