Closed smondet closed 3 years ago
This PR would conflict with #43, but we can probably work things out.
I find the implementation of the decoder too complex. What about just turning the three mutually-recursive functions value
, arr
, obj
into a single function decode
parametrized over an additional state
parameter of type Value of lexeme | Arr of value list | Obj of (string * value) list
?
I find the implementation of the decoder too complex. What about just turning the three mutually-recursive functions value, arr, obj into a single function decode parametrized over an additional state parameter of type Value of lexeme | Arr of value list | Obj of (string * value) list?
I think the problem is non-ability to apply the tail-call optimization on the JavaScript side. I will try to look deeper but the "three mutually-recursive function" should not fit with js_of_ocaml
. @smondet, can you add a JavaScript test to highlight the problem - like try to parse a huge generated JSON object?
To be more precise, my idea is to turn
let rec value v k = match v with
| `Os -> obj [] k
| `As -> arr [] k
| `Null
| `Bool _
| `String _
| `Float _ as v -> k v
| _ -> assert false
and arr vs k = match dec () with
| `Ae -> k (`A (List.rev vs))
| v -> value v (fun v -> arr (v :: vs) k)
and obj ms k = match dec () with
| `Oe -> k (`O (List.rev ms))
| `Name n -> value (dec ()) (fun v -> obj ((n, v) :: ms) k)
| _ -> assert false
into something like
let rec loop f k = match f with
| Value l ->
begin match l with
| `Os -> loop (Obj []) k
| `As -> loop (Arr []) k
| `Null
| `Bool _
| `String _
| `Float _ as v -> k v
| _ -> assert false
end
| Arr vs ->
begin match dec () with
| `Ae -> k (`A (List.rev vs))
| v -> loop (Value v) (fun v -> arr (v :: vs) k)
end
| Obj ms ->
begin match dec () with
| `Oe -> k (`O (List.rev ms))
| `Name n -> loop (Value (dec ())) (fun v -> obj ((n, v) :: ms) k)
| _ -> assert false
end
(or using a GADT if we insist on splitting the function name from its argument)
Yes (if someone has the bandwidth to rewrite it like that :wink: ) I think @gasche's version would be tail-optimized by JSOO.
@dinosaure do you mean tests that depend on some headless-browser and the whole JSOO stack? (the failure is non-optimized tail-calls Vs browser JS engines that have a tiny stack)
This should be closed
(implementing #41 two months later … sorry!)
A couple of additional notes:
value_to_string
in “fatal error” messages (Jsonm validates the input enough that AFAIU those should not happen unless there is bug in the implementation).profile=compact
.This implementation has been running with JSOO for a couple of months in https://tqtezos.github.io/TZComet/ no problem so far.