Allow negative indices <= 0 and indices > 2n in CircularBuffers

[jbrea]

Please ignore this proposition, if you prefer not to allow negative indices and indices > 2n.

[oxinabox]

Thanks for this.
I am not sure if it should be allowed or not.
for circular buffers.
I'ld have to think some more.
(or perhaps someone with experience with these in another language could chime in).

But in anycase,
if this is to be merged it needs to have some tests added.
to prove the functionality works.

Let us know if you have any trouble.

[codecov]

Codecov Report

Merging #451 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #451      +/-   ##
=========================================
+ Coverage   98.79%   98.8%   +<.01%     
=========================================
  Files          27      27              
  Lines        1332    1338       +6     
=========================================
+ Hits         1316    1322       +6     
  Misses         16      16

Impacted Files	Coverage Δ
src/circular_buffer.jl	100% <100%> (ø)	⬆️
src/heaps/binary_heap.jl	100% <0%> (ø)	⬆️
src/heaps/mutable_binary_heap.jl	100% <0%> (ø)	⬆️
src/priorityqueue.jl	99.01% <0%> (+0.02%)	⬆️

---

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 525f990...85b0170. Read the comment docs.

[timholy]

I think the biggest concern is performance, but that "fast exit" at the beginning should help a lot in performance-sensitive cases.

On balance I think it makes sense to make the indexing truly periodic.

[jbrea]

Before

julia> using DataStructures, BenchmarkTools

julia> c = CircularBuffer{Int64}(100);

julia> for i in 1:100 push!(c, i) end

julia> import DataStructures: _buffer_index

julia> @benchmark _buffer_index($c, 40)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     1.953 ns (0.00% GC)
  median time:      1.966 ns (0.00% GC)
  mean time:        1.970 ns (0.00% GC)
  maximum time:     11.326 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @benchmark _buffer_index($c, 140)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     1.634 ns (0.00% GC)
  median time:      1.642 ns (0.00% GC)
  mean time:        1.647 ns (0.00% GC)
  maximum time:     10.826 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

After:

julia> @benchmark _buffer_index($c, 40)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     1.954 ns (0.00% GC)
  median time:      1.969 ns (0.00% GC)
  mean time:        2.246 ns (0.00% GC)
  maximum time:     15.625 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @benchmark _buffer_index($c, 140)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     5.520 ns (0.00% GC)
  median time:      5.539 ns (0.00% GC)
  mean time:        5.602 ns (0.00% GC)
  maximum time:     17.713 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @benchmark _buffer_index($c, -140)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     11.299 ns (0.00% GC)
  median time:      11.521 ns (0.00% GC)
  mean time:        11.802 ns (0.00% GC)
  maximum time:     48.642 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     999

[oxinabox]

This is a little too complex.
Testing the right things,
but a good test set is trivially obvious.

Like

@test cb[1]=1
@test cb[5]=5
@test cb[20]=5
@test cb[0]=5
@test cb[-1]=4
@test cb[-11]=6

Your for j in -19:20 loop does the same thing,
and more comprehensively.
But it is not obvious what it is doing.

Test code that is non-obvious might have bugs.
(and because tests don't have tests ...)

[jbrea]

@oxinabox Thanks for your feedback.
Originally I thought just to change the _buffer_index function, but I guess it makes sense to propagate the changes to getindex and setindex.