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:
|
ml-proto/spec/int.ml
Outdated
| 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 |
ml-proto/spec/int.ml
Outdated
| 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 |
ml-proto/spec/int.ml
Outdated
| | '+' -> 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 |
3 participants
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.