Failing assertion in CCRingBuffer

[copy]

% ocamlbuild -use-ocamlfind -pkg containers.data test.native --
Finished, 4 targets (0 cached) in 00:00:00.
Fatal error: exception Assert_failure("src/data/CCRingBuffer.ml", 330, 6)

It doesn't seem to happen with small buffers, so it's unclear which usage pattern produces this issue. The following reproduces the problem:

module B = CCRingBuffer.Make(struct type t = int end)

let () =
  let b = B.create ~bounded:true 50 in
  while true do
    if Random.float 1.0 < 0.5 then
      B.push_back b 0
    else
      let _ = B.take_front b in ()
  done

[c-cube]

Thanks for the report, I will look into it.

[c-cube]

It's actually possible that I break compatibility a bit, the current code is too complicated…

[copy]

It's actually possible that I break compatibility a bit, the current code is too complicated…

That sounds reasonable. I'd suggest also splitting the module into a bounded and an unbounded version during that process.

[c-cube]

That might be the best, indeed… Or perhaps only split internally but use a phantom type to distinguish:

type 'a t constraints t = [< `bounded|`unbounded]

val capacity : _ t -> int

val max_capacity : [`bounded] t -> int

In any case this is probably the best course of action.

[c-cube]

I removed the unbounded API in ff77a6a, added strong tests, and fixed the bug. However, the fix is pretty basic (some functions like blit_from push items one by one instead of using Array.blit). Now that the base implementation works I can more confidently improve performance.

[struktured]

I always wanted to make blit_from optimized- it would be super easy if the modulars were split too. Let me know if you want me to test anything or contribute in any useful way.

…

On Tue, Jun 13, 2017 at 2:44 PM, Simon Cruanes ***@***.***> wrote: I removed the unbounded API in ff77a6a <ff77a6a>, added strong tests, and fixed the bug. However, the fix is pretty basic (some functions like blit_from push items one by one instead of using Array.blit). Now that the base implementation works I can more confidently improve performance. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#126 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE01ooDrszobuuIgcKgondd6cAoCA_02ks5sDtibgaJpZM4NaMG-> .

[c-cube]

@struktured you are welcome to try. It would be nice, for measuring performance, to put the current version into the benchmarks (as a reference impl) and iterate from there.

With the new tests I'm a bit more confident in the correctness of the module. I think the tests can still be improved (e.g. by supporting more operations in the last random tests), but they're a big step in the right direction.

[c-cube]

Found an issue in skip. These random tests sure are great.

[c-cube]

I'll close this and move to #134 for further changes.