Issue #1351 : bug fix #1396
Issue #1351 : bug fix #1396
Conversation
There was a problem hiding this comment.
it is cool you dig into codegen!
no need patch to stdlib, you only need intercept Parrayrefs and Parraysets https://github.com/bloomberg/bucklescript/blob/master/jscomp/core/lam.ml#L115
the last letter s means safe.
Also the introduced bound checking runtime support is already there, so you don't need add a new one https://github.com/bloomberg/bucklescript/blob/0fc3cd0701079531ca0c04aff361c70318f13994/jscomp/runtime/caml_array.ml#L75
jscomp/stdlib/array.ml
Outdated
| @@ -14,7 +14,7 @@ | |||
| (* Array operations *) | |||
|
|
|||
| external length : 'a array -> int = "%array_length" | |||
| external get: 'a array -> int -> 'a = "%array_safe_get" | |||
| external get: 'a array -> int -> 'a = "caml_array_safe_get" | |||
There was a problem hiding this comment.
you don't need change here, see my comments later
|
@bobzhang Hi finally I fixed all wrong unit tests. I think this is ready. Could you review please? Thanks! |
|
Would you do a rebase against origin/master? |
|
@bobzhang done. |
| var hh; | ||
|
|
||
| try { | ||
| hh = Caml_string.get("ghsogh", -3); |
There was a problem hiding this comment.
hmm, this looks like a regression, will have a look why
There was a problem hiding this comment.
oh, you changed the test code, nvm
| @@ -61,6 +62,31 @@ function caml_array_concat(l) { | |||
| return result; | |||
| } | |||
|
|
|||
| function caml_array_set(xs, index, newval) { | |||
| if (index < 0 || index >= xs.length) { | |||
| throw [ | |||
There was a problem hiding this comment.
@dorafmon would you create a following PR to lift such allocation into the toplevel. such as
let outbound_exn = Invalid_argument "out of bounds"
let caml_array_set xs index new val =
...| function caml_array_get(xs, index) { | ||
| if (index < 0 || index >= xs.length) { | ||
| throw [ | ||
| Caml_builtin_exceptions.invalid_argument, |
There was a problem hiding this comment.
the exception allocation could be shared
|
@bobzhang sure I will send a following PR soon. Let me finish the template string thing first. That is going under a major refactoring at this moment. |
|
hi @dorafmon when you have time, would you mind also check |
|
@bobzhang sure, I think this PR already covers string.get/set, not sure about bytes. I will check later. |
No description provided.