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

fix: spec compliant parseInt #1378

Merged
merged 17 commits into from
Jul 9, 2020
Merged

fix: spec compliant parseInt #1378

merged 17 commits into from
Jul 9, 2020

Conversation

MaxGraey
Copy link
Member

@MaxGraey MaxGraey commented Jul 6, 2020

  • more rejection tests

  • more tests which mixed different prefixes and radices

  • fixes edge cases for parseInt

  • proper generic variants of strtol for F64.parseInt and F32.parseInt

  • I've read the contributing guidelines

std/assembly/util/string.ts Outdated Show resolved Hide resolved
@dcodeIO
Copy link
Member

dcodeIO commented Jul 6, 2020

Iiuc, on Discord it was also reported that global parseInt returns 0 instead of NaN for inputs like "this is not semantically equal to 0". Does this PR fix this as well, does not attempt to fix it, or was this a false alarm?

Example given was

assert(parseInt("this is not semantically equal to 0") == 0)
assert(parseInt("this is not semantically equal to 0") == parseInt("0"))

mentioning that these asserts pass, which seems weird given that global parseInt is

export function parseInt(str: string, radix: i32 = 0): f64 {
  return strtol<f64>(str, radix);
}

@dcodeIO
Copy link
Member

dcodeIO commented Jul 6, 2020

From a quick test it seems that

parseInt("") // returns NaN as expected but
parseInt("abc") // returns 0, which is strange

@MaxGraey
Copy link
Member Author

MaxGraey commented Jul 6, 2020

In my tests parseInt("abc") equal to NaN but not a zero.

See test case below

@MaxGraey
Copy link
Member Author

MaxGraey commented Jul 6, 2020

Iiuc, on Discord it was also reported that global parseInt returns 0 instead of NaN for inputs like "this is not semantically equal to 0". Does this PR fix this as well, does not attempt to fix it, or was this a false alarm?

It fix already halfbacked solution which just not properly tests. So as you suggested in discord solution:

let parsedFloat = parseInt('bad input');
if(isNaN(parsedFloat)) {
   // fail
} else {
  // success
}

Just not worked. Current PR fix this.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 6, 2020

Great, thanks, looks good then with the other comment above addressed :)

@MaxGraey

This comment has been minimized.

@MaxGraey MaxGraey requested a review from dcodeIO July 7, 2020 05:43
@MaxGraey
Copy link
Member Author

MaxGraey commented Jul 7, 2020

Alright. It seems all cases handled properly

@MaxGraey

This comment has been minimized.

@MaxGraey MaxGraey changed the title fix: rejection cases for parseInt and F32/F64.parseInt fix: spec compliant parseInt Jul 7, 2020
std/assembly/util/string.ts Outdated Show resolved Hide resolved
std/assembly/util/string.ts Outdated Show resolved Hide resolved
@dcodeIO dcodeIO merged commit 1d86695 into AssemblyScript:master Jul 9, 2020
@MaxGraey MaxGraey deleted the fix-strtol-for-f64 branch July 9, 2020 20:28
@github-actions
Copy link

🎉 This PR is included in version 0.13.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants