-
Notifications
You must be signed in to change notification settings - Fork 12
Description
I noticed the following four potential problems (unless they are signs of my misunderstanding). I can make a PR for some/all of them if you agree.
-
Line 73 in 9d6a785
t.table <- newt.table;
Shouldn't one also updatet.totsize, byt.totsize <- newt.totsize? -
Lines 110 to 116 in 9d6a785
match Weak.get bucket i with | Some v when v.node = d -> begin match Weak.get bucket i with | Some v -> v | None -> loop (i+1) end | _ -> loop (i+1)
It looks like there is a redundant call toWeak.get(maybe this is due to some limitations in early versions of Weak and its interactions with the GC?). These lines can probably be replaced by:
match Weak.get bucket i with
| Some v when v.node = d -> v
| Some _ | None -> loop (succ i)(the variant when v.hkey = hkey && v.node = d, which allows fast rejection, is probably unnecessary since the equality v.node = d is efficient: this is precisely the purpose of maximal hashconsing).
Line 64 in 9d6a785
let next_sz n = min (3*n/2 + 3) (Sys.max_array_length - 1)
Line 82 in 9d6a785
let newsz = min (3 * sz / 2 + 3) (Sys.max_array_length - 1) in
Both values are capped at Sys.max_array_length - 1, but the first concerns array lengths, so can be capped at Sys.max_array_length, while the second concerns bucket sizes, so should be capped at Obj.Ephemeron.max_ephe_length (which is currently Sys.max_array_length - 2). Also, is the growth rate purposefully, or accidentally the same?
- Most integer equality checks in the file use
==(although some use=, with no clear pattern of when which is used). Why not use=everywhere? OrInt.equal?