Write integer of_string from scratch #283
Conversation
While WebAssembly#265 did the job of supporting unsigned integers, it certainly was no nice implementation. This version adds the same features, i.e. allowing signed integers (as defined in the lexer) and larger unsigned values, but is actually written from scratch and doesn't rely on the builtin of_string anymore. This PR should close WebAssembly#225 if it is accepted.
|
Hm, this code seems to cause assertion failures on invalid input. That should never happen. Can you change it to raise normal exceptions, preferably the same as the original? Additional nit: there are plenty of redundant parens in the code. |
|
I removed all redundent parentheses, that I could find, and added a "try" to catch my asserts. |
|
Please do not use assertions like this. Assertions are intended for checking invariants (pre/post-conditions) and thus the internal consistency of code, not for checking the validity of input data. An assertion failure is supposed to signal a program bug. Also, please do never write catch-all try handlers. That is a guarantee for unintentionally masking unrelated errors. Only match the exceptions you expect to get and intend to handle. |
|
Oh, sorry, I didn't know that. I hope, that I solved it correctly. |
| let of_string = Rep.of_string | ||
| (* This implementation allows leading signs and unsigned values *) | ||
| let of_string x = | ||
| let len = String.length x in |
There was a problem hiding this comment.
Hint: let open Rep in allows to omit the repeated module prefix.
|
Thanks! I left a few comments. Also, it would be good to have a few additional tests, especially for boundary conditions. |
|
I addressed the comments, but there's some stuff I'm not sure about:
|
| else failwith "unexpected digit" | ||
|
|
||
| let neg_safe x = | ||
| if lt_s (sub x Rep.one) Rep.minus_one then raise Numerics.IntegerOverflow; |
There was a problem hiding this comment.
Since this is called only once, I'd just inline it. And just use failwith, too.
|
Thanks, looks pretty good now, just a few nits left. As for testing, you can write test files named |
| if '0' <= c && '9' >= c then 0x30 | ||
| else if 'a' <= c && 'f' >= c then 0x61 - 10 | ||
| else if 'A' <= c && 'F' >= c then 0x41 - 10 | ||
| else failwith "unexpected digit" |
There was a problem hiding this comment.
fwiw, I find the following easier to read :
let parse_hexdigit = function
| '0' .. '9' as c -> Char.code c - Char.code '0'
| 'a' .. 'f' as c -> 0xa + Char.code c - Char.code 'a'
| 'A' .. 'F' as c -> 0xa + Char.code c - Char.code 'A'
| _ -> failwith "unexpected digit"
There was a problem hiding this comment.
I think, this is a lot prettier than my version @rossberg-chromium shall I copy this?
|
I added some tests and hoisted |
| @@ -250,7 +249,12 @@ struct | |||
| in | |||
| match x.[0] with | |||
| | '+' -> parse_int 1 | |||
| | '-' -> neg_safe (parse_int 1) | |||
| | '-' -> | |||
| begin | |||
There was a problem hiding this comment.
Nit: you don't need the begin/end here
|
Thanks! LGTM modulo a last couple of nits |
Decided to cheat a little bit, and squeeze v8x16.swizzle into the i8x16 ast node, so that we don't have to create a new v8x16 data type. Semantically it means the same thing, v8x16 is called that way since it doesn't treat the underlying data as any particular type, so treating it as int will work too.
* Reintroduce exnref * Fix type_of * Simplify parser * Implement 1b * Change opcodes for catch/catch_all to avoid conflict * Put catch clauses first * Remove obsolete Delegating cases * Change exn type opcode to -0x17 * Switch to B' variant * [interpreter] Add boilerplate for ref.exn result patterns * [ci] Deactivate node run, since it can't handle try_table yet * Try -> TryTable in AST * [spec] Update spec for option B' (#283) * Deactivate Bikeshed * [spec/tests] Specification of legacy exceptions + tests (#284) * [legacy] Create specification doc for legacy exception handling * [test] Create infra for legacy tests
While #265 did the job of supporting unsigned integers, it certainly was
no nice implementation. This version adds the same features, i.e.
allowing signed integers (as defined in the lexer) and larger unsigned
values, but is actually written from scratch and doesn't rely on the
builtin of_string anymore. This PR should close #225 if it is accepted.