-
Notifications
You must be signed in to change notification settings - Fork 478
Add support for unsigned decimals #265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
(Rep.mul | ||
(Rep.of_string (String.sub x 0 (l - 1))) | ||
(Rep.of_int 10)) | ||
(Rep.of_string (String.sub x (l - 1) 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code silently wraps on overflow (unless it runs out of stack space first ;-)). I think the spec should explicitly diagnose overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't think of the overflow, thanks! But how could the stack run out of space?
I added some overflow-tests and added the possibility to have a plus-sign in front of numbers, which was defined in host/lexer.mll, but is not allowed by the builtin of_string function. |
let ten = Rep.of_int 10 in | ||
let l = String.length x in | ||
(* First overflow-check via String.length *) | ||
if l > (if Rep.bitwidth = 32 then 10 else 20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this effectively hard-codes that we only instantiate with 32 and 64 bit. It would be more modular to have a generic criterium, e.g. bitwidth / 3
.
(Sorry for the delay, just rediscovered this one). I think this is good to have, but the implementation is a bit more ad-hoc than I'd hope. Wouldn't it be simpler to just reimplement the normal conversion from scratch? |
I think that would be a good alternative, this code may work, but its probably not a nice solution. |
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.
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 passes simd_i8x16_arith2.wast.
) This addresses WebAssembly/exception-handling#253 (comment) by adding a set of tests that use a Wasm-defined and exported tag.
This fixes #225