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

Fix an implementation/specification mismatch in BatVect.insert #767

Merged
merged 1 commit into from
Aug 3, 2017

Conversation

gasche
Copy link
Member

@gasche gasche commented Jul 30, 2017

BatVect.insert : int -> 'a t -> 'a t is specified so that insert n r u inserts the rope r between the elements of index n and n+1 in u. Its implementation does something different, it inserts r strictly before the element of index n in u.

The present commit changes the specification to match the implementation.

First, we expect users to have tested their program instead of trusting (or, unfortunately, even reading) the documentation, so it is likely that the uses of BatVect.insert with the current implementation is correct for their needs; changing it would break user program.

Second, we argue that the implemented behavior is actually far more
natural:

  • the implementation (in terms of sub and concat) is nicer -- hear the code!

  • the ranges of integer values valid for insert is 0 .. (length u - 1), as one would expect, instead of (-1) .. (length u - 2) as with the current specification.

  • this gives a very natural invariant between insert and BatVect.remove : (*start*)int -> (*len*)int -> 'a t -> 'a t: u is equal to u |> insert n r |> remove n (length r)).

fixes #766.

@UnixJunkie
Copy link
Member

I get this when I do 'make test':

File "src/batArray.ml", line 179, characters 40-51:
Error: This expression has type int Q.arbitrary = int QCheck.arbitrary
       but an expression was expected of type
	    'a Q.Observable.t = 'a QCheck.Observable.t

I am using qtest.2.5.

@c-cube
Copy link
Member

c-cube commented Jul 31, 2017 via email

@gasche
Copy link
Member Author

gasche commented Jul 31, 2017

Currently the tests in Batteries require (qtest >= 2.0 & qtest < 2.5), see the opam file.

@gasche gasche merged commit 489e276 into ocaml-batteries-team:master Aug 3, 2017
`BatVect.insert : int -> 'a t -> 'a t` is specified so that `insert
n r u` inserts the rope `r` between the elements of index `n` and
`n+1` in `u`. Its implementation does something different, it inserts
`r` strictly before the element of index `n` in `u`.

The present commit changes the specification to match the
implementation.

First, we expect users to have tested their program instead of
trusting (or, unfortunately, even reading) the documentation, so it is
likely that the uses of BatVect.insert with the current implementation
is correct for their needs; changing it would break user program.

Second, we argue that the implemented behavior is actually far more
natural:

- the implementation (in terms of `sub` and `concat`) is nicer -- hear
  the code!

- the ranges of integer values valid for `insert` is
  `0  .. (length u - 1)`, as one would expect, instead of
  `(-1) .. (length u - 2)` as with the current specification.

- this gives a very natural invariant between `insert` and
  `BatVect.remove : (*start*)int -> (*len*)int -> 'a t -> 'a t`:
  `u` is equal to `u |> insert n r |> remove n (length r))`.

fixes ocaml-batteries-team#766.
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.

Bug: off-by-one in BatVect.insert
3 participants