From f90f73f671dc82af197d010f4ed23c7dda033e24 Mon Sep 17 00:00:00 2001
From: Leonid Rozenberg <leonidr@gmail.com>
Date: Wed, 19 Apr 2017 12:26:47 -0400
Subject: [PATCH] Incorporate reviewier feedback.

Also added style elements from PR#116.
---
 src/data/CCBV.ml  | 212 ++++++++++++++++++++++++++--------------------
 src/data/CCBV.mli |  58 ++++++++-----
 2 files changed, 160 insertions(+), 110 deletions(-)

diff --git a/src/data/CCBV.ml b/src/data/CCBV.ml
index 144661d4..179e482b 100644
--- a/src/data/CCBV.ml
+++ b/src/data/CCBV.ml
@@ -1,29 +1,30 @@
 
 (** {2 Imperative Bitvectors} *)
 
-let __width = Sys.word_size - 1
+let width_ = Sys.word_size - 1
 
 (** We use OCamls ints to store the bits. We index them from the
     least significant bit. We create masks to zero out the most significant
-    bits that aren't used to store values. *)
-let __lsb_masks =
-  let a = Array.make (__width + 1) 0 in
-  for i = 1 to __width do
+    bits that aren't used to store values. This is necessary when we are
+    constructing or negating a bit vector. *)
+let lsb_masks_ =
+  let a = Array.make (width_ + 1) 0 in
+  for i = 1 to width_ do
     a.(i) <- a.(i-1) lor (1 lsl (i - 1))
   done;
   a
 
-let __all_ones = __lsb_masks.(__width)
+let all_ones_ = lsb_masks_.(width_)
 
 (* count the 1 bits in [n]. See https://en.wikipedia.org/wiki/Hamming_weight *)
-let __count_bits n =
+let count_bits_ n =
   let rec recurse count n =
     if n = 0 then count else recurse (count+1) (n land (n-1))
   in
   recurse 0 n
 
 (*  Can I access the "private" members in testing? $Q
-  (Q.int_bound (Sys.word_size - 1)) (fun i -> __count_bits __lsb_masks.(i) = i)
+  (Q.int_bound (Sys.word_size - 1)) (fun i -> count_bits_ lsb_masks_.(i) = i)
 *)
 
 type t = {
@@ -35,23 +36,24 @@ let length t = t.size
 
 let empty () = { a = [| |] ; size = 0 }
 
-let __to_array_legnth size =
-  if size mod __width = 0 then size / __width else (size / __width) + 1
+let array_length_of_size size =
+  if size mod width_ = 0 then size / width_ else (size / width_) + 1
 
 let create ~size default =
-  if size = 0 then { a = [| |] ; size }
-  else begin
-    let n = __to_array_legnth size in
-    let arr = if default
-      then Array.make n __all_ones
+  if size = 0 then { a = [| |]; size }
+  else (
+    let n = capa_of_size size in
+    let a = if default
+      then Array.make n all_ones_
       else Array.make n 0
     in
     (* adjust last bits *)
-    let r = size mod __width in
-    if default && r <> 0
-    then Array.unsafe_set arr (n-1) __lsb_masks.(r);
-    { a = arr; size }
-  end
+    let r = size mod width_ in
+    if default && r <> 0 then (
+      Array.unsafe_set a (n-1) lsb_masks_.(r);
+    );
+    { a; size }
+  )
 
 (*$Q
   (Q.pair Q.small_int Q.bool) (fun (size, b) -> create ~size b |> length = size)
@@ -65,52 +67,69 @@ let create ~size default =
   create ~size:29 true |> to_sorted_list = CCList.range 0 28
 *)
 
-let copy bv = { a = Array.copy bv.a ; size = bv.size }
+let copy bv = { bv with a = Array.copy bv.a }
 
 (*$Q
   (Q.list Q.small_int) (fun l -> \
     let bv = of_list l in to_list bv = to_list (copy bv))
 *)
 
-let capacity bv = __width * Array.length bv.a
+let capacity bv = width_ * Array.length bv.a
+
+(* iterate on words of width (at most) [width_] *)
+let iter_words ~f bv: unit =
+  if bv.size = 0 then ()
+  else (
+    let len = array_length_of_size bv.size in
+    assert (len>0);
+    for i = 0 to len-1 do
+      let word = Array.unsafe_get a i in
+      f i word
+    done;
+    if r <> 0 then f (len-1) (Array.unsafe_get a (len-1) land lsb_masks_.(r));
+  )
 
 let cardinal bv =
-  let n = ref 0 in
-  for i = 0 to Array.length bv.a - 1 do
-    n := !n + __count_bits bv.a.(i)
-  done;
-  !n
+  if bv.size = 0 then 0
+  else (
+    let n = ref 0 in
+    for i = 0 to Array.length bv.a - 1 do
+      n := !n + count_bits_ bv.a.(i)         (* MSB of last element are all 0 *)
+    done;
+    !n
+  )
 
 (*$Q
   Q.small_int (fun size -> create ~size true |> cardinal = size)
 *)
 
-let __really_resize bv ~desired ~current size =
+let really_resize_ bv ~desired ~current size =
   let a' = Array.make desired 0 in
   Array.blit bv.a 0 a' 0 current;
   bv.a <- a';
   bv.size <- size
 
-let __grow bv size =
+let grow_ bv size =
   if size <= capacity bv                                   (* within capacity *)
   then bv.size <- size
-  else                                                     (* beyond capacity *)
-    let desired = __to_array_legnth size in
+  else (                                                   (* beyond capacity *)
+    let desired = array_length_of_size size in
     let current = Array.length bv.a in
-    __really_resize bv ~desired ~current size
+    really_resize_ bv ~desired ~current size
+  )
 
-let __shrink bv size =
-  let desired = __to_array_legnth size in
+let shrink_ bv size =
+  let desired = array_length_of_size size in
   let current = Array.length bv.a in
-  __really_resize bv ~desired ~current size
+  really_resize_ bv ~desired ~current size
 
 let resize bv size =
   if size < 0 then invalid_arg "resize: negative size" else
   if size < bv.size                                                 (* shrink *)
-  then __shrink bv size
+  then shrink_ bv size
   else if size = bv.size
   then ()
-  else __grow bv size
+  else grow_ bv size
 
 (*$R
   let bv1 = CCBV.create ~size:87 true in
@@ -124,19 +143,19 @@ let resize bv size =
 let is_empty bv =
   try
     for i = 0 to Array.length bv.a - 1 do
-      if bv.a.(i) <> 0 then raise Exit
+      if bv.a.(i) <> 0 then raise Exit       (* MSB of last element are all 0 *)
     done;
     true
   with Exit ->
     false
 
 let get bv i =
-  if i < 0 then invalid_arg "get: negative index" else
-    let n = i / __width in
-    let i = i mod __width in
-    if n < Array.length bv.a
-    then (Array.unsafe_get bv.a n) land (1 lsl i) <> 0
-    else false
+  if i < 0 then invalid_arg "get: negative index";
+  let n = i / width_ in
+  let i = i mod width_ in
+  if n < Array.length bv.a
+  then (Array.unsafe_get bv.a n) land (1 lsl i) <> 0
+  else false
 
 (*$R
   let bv = CCBV.create ~size:99 false in
@@ -155,11 +174,13 @@ let get bv i =
 *)
 
 let set bv i =
-  if i < 0 then invalid_arg "set: negative index" else
-    let n = i / __width in
-    let j = i mod __width in
-    if i >= bv.size then __grow bv i;
+  if i < 0 then invalid_arg "set: negative index"
+  else (
+    let n = i / width_ in
+    let j = i mod width_ in
+    if i >= bv.size then grow_ bv i;
     Array.unsafe_set bv.a n ((Array.unsafe_get bv.a n) lor (1 lsl j))
+  )
 
 (*$T
   let bv = create ~size:3 false in set bv 0; get bv 0
@@ -167,36 +188,40 @@ let set bv i =
 *)
 
 let reset bv i =
-  if i < 0 then invalid_arg "reset: negative index" else
-    let n = i / __width in
-    let j = i mod __width in
-    if i >= bv.size then __grow bv i;
+  if i < 0 then invalid_arg "reset: negative index"
+  else (
+    let n = i / width_ in
+    let j = i mod width_ in
+    if i >= bv.size then grow_ bv i;
     Array.unsafe_set bv.a n ((Array.unsafe_get bv.a n) land (lnot (1 lsl j)))
+  )
 
 (*$T
   let bv = create ~size:3 false in set bv 0; reset bv 0; not (get bv 0)
 *)
 
 let flip bv i =
-  if i < 0 then invalid_arg "reset: negative index" else
-    let n = i / __width in
-    let j = i mod __width in
-    if i >= bv.size then __grow bv i;
+  if i < 0 then invalid_arg "reset: negative index"
+  else (
+    let n = i / width_ in
+    let j = i mod width_ in
+    if i >= bv.size then grow_ bv i;
     Array.unsafe_set bv.a n ((Array.unsafe_get bv.a n) lxor (1 lsl j))
+  )
 
 (*$R
   let bv = of_list [1;10; 11; 30] in
   flip bv 10;
-  assert_equal [1;11;30] (to_sorted_list bv);
-  assert_equal false (get bv 10);
+  assert_equal ~printer:Q.Print.(list int) [1;11;30] (to_sorted_list bv);
+  assert_equal ~printer:Q.Print.bool false (get bv 10);
   flip bv 10;
-  assert_equal true (get bv 10);
+  assert_equal ~printer:Q.Print.bool true (get bv 10);
   flip bv 5;
-  assert_equal [1;5;10;11;30] (to_sorted_list bv);
-  assert_equal true (get bv 5);
+  assert_equal ~printer:Q.Print.(list int) [1;5;10;11;30] (to_sorted_list bv);
+  assert_equal ~printer:Q.Print.bool true (get bv 5);
   flip bv 100;
-  assert_equal [1;5;10;11;30;100] (to_sorted_list bv);
-  assert_equal true (get bv 100);
+  assert_equal ~printer:Q.Print.(list int) [1;5;10;11;30;100] (to_sorted_list bv);
+  assert_equal ~printer:Q.Print.bool true (get bv 100);
 *)
 
 let clear bv =
@@ -216,11 +241,17 @@ let clear bv =
 
 let iter bv f =
   let len = Array.length bv.a in
-  for n = 0 to len - 1 do
-    let j = __width * n in
-    for i = 0 to __width - 1 do
+  for n = 0 to len - 2 do
+    let j = width_ * n in
+    for i = 0 to width_ - 1 do
       f (j+i) (bv.a.(n) land (1 lsl i) <> 0)
     done
+  done;
+  let j = max 0 (width_ * (len - 2)) in
+  let r = size mod width_ in
+  let final_length = if r = 0 then width_ else r in
+  for i = 0 to final_length - 1 do
+    f (j + i) (bv.a.(len - 1) land (i lsl i) <> 0)
   done
 
 (*$R
@@ -232,14 +263,7 @@ let iter bv f =
 *)
 
 let iter_true bv f =
-  let len = Array.length bv.a in
-  for n = 0 to len - 1 do
-    let j = __width * n in
-    for i = 0 to __width - 1 do
-      if bv.a.(n) land (1 lsl i) <> 0
-      then f (j+i)
-    done
-  done
+  iter bv (fun i b -> if b then f i else ())
 
 (*$T
   of_list [1;5;7] |> iter_true |> Sequence.to_list |> List.sort CCOrd.compare = [1;5;7]
@@ -294,15 +318,19 @@ let of_list l =
 
 exception FoundFirst of int
 
-let first bv =
+let first_exn bv =
   try
     iter_true bv (fun i -> raise (FoundFirst i));
     raise Not_found
   with FoundFirst i ->
     i
 
+let first bv =
+  try Some (first_exn bv)
+  with Not_found -> None
+
 (*$T
-  of_list [50; 10; 17; 22; 3; 12] |> first = 3
+  of_list [50; 10; 17; 22; 3; 12] |> first = Some 3
 *)
 
 let filter bv p =
@@ -319,10 +347,10 @@ let negate_self b =
   for n = 0 to len - 1 do
     Array.unsafe_set b.a n (lnot (Array.unsafe_get b.a n))
   done;
-  let r = b.size mod __width in
+  let r = b.size mod width_ in
   if r <> 0 then
     let l = Array.length b.a - 1 in
-    Array.unsafe_set b.a l (__lsb_masks.(r) land (Array.unsafe_get b.a l))
+    Array.unsafe_set b.a l (lsb_masks_.(r) land (Array.unsafe_get b.a l))
 
 (*$T
   let v = of_list [1;2;5;7;] in negate_self v; \
@@ -331,10 +359,10 @@ let negate_self b =
 
 let negate b =
   let a =  Array.map (lnot) b.a in
-  let r = b.size mod __width in
+  let r = b.size mod width_ in
   if r <> 0 then begin
     let l = Array.length b.a - 1 in
-    Array.unsafe_set a l (__lsb_masks.(r) land (Array.unsafe_get a l))
+    Array.unsafe_set a l (lsb_masks_.(r) land (Array.unsafe_get a l))
   end;
   { a ; size = b.size }
 
@@ -345,7 +373,7 @@ let negate b =
 (* Underlying size grows for union. *)
 let union_into ~into bv =
   if into.size < bv.size
-  then __grow into bv.size;
+  then grow_ into bv.size;
   for i = 0 to (Array.length into.a) - 1 do
     Array.unsafe_set into.a i
       ((Array.unsafe_get into.a i) lor (Array.unsafe_get bv.a i))
@@ -354,21 +382,21 @@ let union_into ~into bv =
 (* To avoid potentially 2 passes, figure out what we need to copy. *)
 let union b1 b2 =
   if b1.size <= b2.size
-  then begin
+  then (
     let into = copy b2 in
     for i = 0 to (Array.length b1.a) - 1 do
       Array.unsafe_set into.a i
         ((Array.unsafe_get into.a i) lor (Array.unsafe_get b1.a i))
     done;
     into
-  end else begin
+  ) else (
     let into = copy b1 in
     for i = 0 to (Array.length b1.a) - 1 do
       Array.unsafe_set into.a i
         ((Array.unsafe_get into.a i) lor (Array.unsafe_get b2.a i))
     done;
     into
-  end
+  )
 
 (*$R
   let bv1 = CCBV.of_list [1;2;3;4] in
@@ -386,7 +414,7 @@ let union b1 b2 =
 (* Underlying size shrinks for inter. *)
 let inter_into ~into bv =
   if into.size > bv.size
-  then __shrink into bv.size;
+  then shrink_ into bv.size;
   for i = 0 to (Array.length into.a) - 1 do
     Array.unsafe_set into.a i
       ((Array.unsafe_get into.a i) land (Array.unsafe_get bv.a i))
@@ -394,21 +422,21 @@ let inter_into ~into bv =
 
 let inter b1 b2 =
   if b1.size <= b2.size
-  then begin
+  then (
     let into = copy b1 in
     for i = 0 to (Array.length b1.a) - 1 do
       Array.unsafe_set into.a i
         ((Array.unsafe_get into.a i) land (Array.unsafe_get b2.a i))
     done;
     into
-  end else begin
+  ) else (
     let into = copy b2 in
     for i = 0 to (Array.length b2.a) - 1 do
       Array.unsafe_set into.a i
         ((Array.unsafe_get into.a i) land (Array.unsafe_get b1.a i))
     done;
     into
-  end
+  )
 
 (*$T
   inter (of_list [1;2;3;4]) (of_list [2;4;6;1]) |> to_sorted_list = [1;2;4]
@@ -431,7 +459,7 @@ let diff_into ~into bv =
       ((Array.unsafe_get into.a i) land (lnot (Array.unsafe_get bv.a i)))
   done
 
-let diff ~in_ not_in =
+let diff in_ not_in =
   let into = copy in_ in
   diff_into ~into not_in;
   into
@@ -474,10 +502,12 @@ let selecti bv arr =
   assert_equal [("b",1); ("c",2); ("f",5)] l;
 *)
 
-(*$T
+(*$= & ~printer:Q.Print.(list (pair int int))
   selecti (of_list [1;4;3]) [| 0;1;2;3;4;5;6;7;8 |] \
+  [1,1; 3,3; 4,4] (selecti (of_list [1;4;3]) [| 0;1;2;3;4;5;6;7;8 |] \
     |> List.sort CCOrd.compare = [1, 1; 3,3; 4,4]
-*)
+    |> List.sort CCOrd.compare)
+ *)
 
 type 'a sequence = ('a -> unit) -> unit
 
diff --git a/src/data/CCBV.mli b/src/data/CCBV.mli
index 414d50c2..4d0e4bae 100644
--- a/src/data/CCBV.mli
+++ b/src/data/CCBV.mli
@@ -3,9 +3,13 @@
 
 (** {2 Imperative Bitvectors}
 
-    The size of the bitvector is rounded up to the multiple of 30 or  62.
-    In other words some functions such as {!iter} might iterate on more
-    bits than what was originally asked for.
+    {b BREAKING CHANGES} since NEXT_RELEASE:
+    size is now stored along with the bitvector. Some functions have
+    a new signature.
+
+    The size of the bitvector used to be rounded up to the multiple of 30 or 62.
+    In other words some functions such as {!iter} would iterate on more
+    bits than what was originally asked for. This is not the case anymore.
 *)
 
 type t
@@ -21,13 +25,18 @@ val copy : t -> t
 (** Copy of bitvector *)
 
 val cardinal : t -> int
-(** Number of set bits. *)
+(** Number of bits set to one, seen as a set of bits. *)
 
 val length : t -> int
-(** Length of underlying bitvector. *)
+(** Size of underlying bitvector.
+    This is not related to the underlying implementation.
+    Changed at NEXT_RELEASE
+*)
 
 val capacity : t -> int
-(** The number of bits this bitvector can store without resizing. *)
+(** The number of bits this bitvector can store without resizing.
+
+    @since NEXT_RELEASE *)
 
 val resize : t -> int -> unit
 (** Resize the BV so that it has the specified length. This can grow or shrink
@@ -36,19 +45,19 @@ val resize : t -> int -> unit
     @raise Invalid_arg on negative sizes. *)
 
 val is_empty : t -> bool
-(** Any bit set? *)
+(** Are there any true bits? *)
 
 val set : t -> int -> unit
-(** Set i-th bit. *)
+(** Set i-th bit, extending the bitvector if needed. *)
 
 val get : t -> int -> bool
 (** Is the i-th bit true? Returns false if the index is too high*)
 
 val reset : t -> int -> unit
-(** Set i-th bit to 0 *)
+(** Set i-th bit to 0, extending the bitvector if needed. *)
 
 val flip : t -> int -> unit
-(** Flip i-th bit *)
+(** Flip i-th bit, extending the bitvector if needed. *)
 
 val clear : t -> unit
 (** Set every bit to 0 *)
@@ -72,16 +81,23 @@ val of_list : int list -> t
     The bits are interpreted as indices into the returned bitvector, so the final
     bitvector will have [length t] equal to 1 more than max of list indices. *)
 
-val first : t -> int
-(** First set bit, or
-    @raise Not_found if all bits are 0 *)
+val first : t -> int option
+(** First set bit, or return None.
+    changed type at NEXT_RELEASE *)
+
+val first_exn : t -> int
+ (** First set bit, or
+     @raise Not_found if all bits are 0
+     @since NEXT_RELEASE *)
 
 val filter : t -> (int -> bool) -> unit
 (** [filter bv p] only keeps the true bits of [bv] whose [index]
     satisfies [p index] *)
 
 val negate_self : t -> unit
-(** [negate_self t] flips all of the bits in [t]. *)
+(** [negate_self t] flips all of the bits in [t].
+
+    @since NEXT_RELEASE *)
 
 val negate : t -> t
 (** [negate t] returns a copy of [t] with all of the bits flipped. *)
@@ -89,12 +105,12 @@ val negate : t -> t
 val union_into : into:t -> t -> unit
 (** [union ~into bv] sets [into] to the union of itself and [bv].
 
-    Note that [into] will grow to accammodate the union. *)
+    Also updates the length of [into] to be at least [length bv]. *)
 
 val inter_into : into:t -> t -> unit
 (** [inter ~into bv] sets [into] to the intersection of itself and [bv]
 
-    Note that [into] will shrink to accammodate the union. *)
+    Also updates the length of [into] to be at most [length bv]. *)
 
 val union : t -> t -> t
 (** [union bv1 bv2] returns the union of the two sets *)
@@ -103,10 +119,14 @@ val inter : t -> t -> t
 (** [inter bv1 bv2] returns the intersection of the two sets *)
 
 val diff_into : into:t -> t -> unit
-(** [diff ~into t] Modify [into] with only the bits set but not in [t]. *)
+(** [diff ~into t] Modify [into] with only the bits set but not in [t].
 
-val diff : in_:t -> t -> t
-(** [diff ~in_ t] Return those bits found [in_] but not in [t]. *)
+    @since NEXT_RELEASE *)
+
+val diff : t -> t -> t
+(** [diff t1 t2] Return those bits found [t1] but not in [t2].
+
+    @since NEXT_RELEASE *)
 
 val select : t -> 'a array -> 'a list
 (** [select arr bv] selects the elements of [arr] whose index