add CCVector.resize_with and CCVector.resize_with_init, tests and doc #389
Conversation
src/core/CCVector.ml
Outdated
| @@ -201,6 +201,52 @@ let push v x = | |||
| let v = of_list [1;2;3] in push v 4; to_list v = [1;2;3;4] | |||
| *) | |||
|
|
|||
| let resize_with v f size = | |||
| if size > Sys.max_array_length then | |||
| failwith "vec.resize_with: size too big" | |||
There was a problem hiding this comment.
Agreed, invalid_argument "…" is more in line with the rest 🙂
src/core/CCVector.ml
Outdated
| else | ||
| let new_vec = Array.make size (f 0) in | ||
| for i = 0 to size - 1 do | ||
| Array.unsafe_set new_vec i (f i) | ||
| done; | ||
| v.vec <- new_vec; | ||
| v.size <- size; | ||
| () |
There was a problem hiding this comment.
| else | |
| let new_vec = Array.make size (f 0) in | |
| for i = 0 to size - 1 do | |
| Array.unsafe_set new_vec i (f i) | |
| done; | |
| v.vec <- new_vec; | |
| v.size <- size; | |
| () | |
| else ( | |
| let new_vec = Array.make size (f 0) in | |
| for i = v.size to size - 1 do | |
| Array.unsafe_set new_vec i (f i) | |
| done; | |
| v.vec <- new_vec; | |
| v.size <- size; | |
| () | |
| ) |
The semantics should be, imho, that f is only called for new arguments. In particular, resize_with v f (size v) should not do anything.
src/core/CCVector.ml
Outdated
| if size > Sys.max_array_length then | ||
| failwith "vec.resize_with_init: size too big" | ||
| else if size <= Array.length v.vec then | ||
| for i = 0 to v.size - 1 do |
There was a problem hiding this comment.
same remark here, don't touch the existing elements
src/core/CCVector.mli
Outdated
| (** [resize_with vec f size] resizes vector [vec] up to [size], fills vector | ||
| with calls to [f] on indexes [[0.. size - 1]]. | ||
| The contents of [vec] are merely overwritten with calls to [f] if [size] | ||
| is inferior or equal to the current size of [vec]. |
There was a problem hiding this comment.
both the docs could mention @raise Invalid_argument if the size is too big
|
Thank you for your reviews @c-cube I've applied the changes you have suggested, I have yet to commit them, though I have a question regarding 1- Should I also fail if Not raising |
|
I'm not sure, tbh. Doing the check ourselves helps giving better error messages, but it's also a tiny bit of overhead. I think it's fine skipping them altogether. |
src/core/CCVector.ml
Outdated
| let resize_with v f size = | ||
| if size > Sys.max_array_length then | ||
| invalid_arg "vec.resize_with: size too big" | ||
| else if size > Array.length v.vec then |
There was a problem hiding this comment.
Still not correct, this will not work if, say, v.size = 5 but Array.length v.vec = 10 when we want to resize to 8.
I think it's simpler to call the function to ensure capacity is >= size, and then add missing elements.
src/core/CCVector.ml
Outdated
| let resize_with_init v ~init size = | ||
| if size > Sys.max_array_length then | ||
| invalid_arg "vec.resize_with_init: size too big" | ||
| else if size > Array.length v.vec then |
There was a problem hiding this comment.
Same, this doesn't do anything in case v.vec is big enough but v.size < size.
src/core/CCVector.ml
Outdated
|
|
||
| (*$T | ||
| let v = make 1 0 in to_list (resize_with_init v ~init:1 5) = [1;1;1;1;1] | ||
| let v = make 1 0 in length (resize_with_init v ~init:1 5) = 5 |
There was a problem hiding this comment.
need some tests to cover the case mentioned above :)
|
Hello again, I think i've fixed the bug you mentioned, i've added a single test in each function in regards to it |
|
Did you run the tests locally? I suspect they don't compile (there's a edit: to run the tests you need qtest, qcheck, and |
|
Yikes, my bad I was running |
|
|
|
I'm going to merge and fix on master. |
Greetings,
I've been using this lib for a school project of mine and it has been of great help.
I've come to need the feature this PR adds so I thought it would benefit other people too.
It being my first contribution to the lib, do tell me if I have omitted anything.
Cordially :)