Skip to content
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

Revise simple number tagging and untagging throughout the stdlib #1072

Open
cician opened this issue Dec 11, 2021 · 0 comments
Open

Revise simple number tagging and untagging throughout the stdlib #1072

cician opened this issue Dec 11, 2021 · 0 comments

Comments

@cician
Copy link
Contributor

cician commented Dec 11, 2021

Throughout the entire standard library we have some possibly problematic usage of simple number tagging and untagging.

  1. In some if not most cases where a WasmI32 value is "tagged" into a SImple number, we should use conv.wasmI32ToNumber instead of "tagging" it directly because a range of possible WasmI32 numbers don't fit into Simple numbers. Issue String.byteLength incorrect for strings larger than 2GB #1064 is an example of this. We can review case by case if direct tagging is OK or not in order to avoid a performance regressions, but I think conv.wasmI32ToNumber is fast enough that we can just use it everywhere.

  2. There seem to be many cases of ad-hoc tagging and untagging without even calling a function like tagSimpleNumber. Other than the possibility of above mentioned problems, I think it makes code too cryptic, so we may change it even if direct tagging or untagging is justified. Here's a list of places that seem to have some raw tagging/untagging logic like this. It's just a result of quick text search of shift operators, so probably not accurate or exhaustive.

  • runtime/wasi.gr
    stringOfSystemError

  • map.gr
    toArray

  • buffer.gr
    addStringSlice

  • array.gr
    initLength

  • string.gr
    grainToWasmNumber
    charAt
    explodeHelp
    split
    slice
    contains
    utf16Length
    encodeAtHelp
    encodeHelp

  • runtime/string.gr
    getVariantName
    toStringHelp
    listToString
    parseInt

  • sys/process.gr
    exit
    sigRaise

  • sys/file.gr
    Too many to list. Just search >> 1n.

  1. We may want to unify runtime.dataStructures.tagSimpleNumber and runtime.numbers.tagSimple functions. To avoid braking changes maybe one could reexport the other. If there are justified uses of direct untagging instead of calling coerceNumberToWasmI32, then we may want to export runtime.numbers.untagSimple and use this instead of ad-hoc conversion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants