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 printing negative zero in JS backend #16505

Merged
merged 1 commit into from
Dec 29, 2020
Merged

fix printing negative zero in JS backend #16505

merged 1 commit into from
Dec 29, 2020

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 29, 2020

see timotheecour#136

before this doesn't work in JS backend(echo $a prints 0.0 instead of -0.0).

proc main()=
  block:
    let a = -0.0
    doAssert $a == "-0.0"
    doAssert $(-0.0) == "-0.0"


static: main()
main()
  • VM prints 0.0 as -0.0, I will fix it in the following PR.

Copy link
Member Author

@ringabout ringabout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Araq Araq merged commit 89a2390 into nim-lang:devel Dec 29, 2020
@ringabout
Copy link
Member Author

ringabout commented Dec 29, 2020

Unfortunately, the VM bug is not easy to solve

Uncomment the codes cause the memory of y is polluted

proc main()=
  # block:
  #   let x = -0.0
  #   doAssert $x == "-0.0"

  block:
    let y = 0.0
    doAssert $y == "0.0"


static: main()

What's your opinion? @timotheecour

asm """
function nimOnlyDigitsOrMinus(n) {
return n.toString().match(/^-?\d+$/);
}
if (Number.isSafeInteger(`a`)) `result` = `a`+".0"
if (Number.isSafeInteger(`a`))
`result` = `a` === 0 && 1 / `a` < 0 ? "-0.0" : `a`+".0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xflywind I just found out about Object.is, we should use it instead, it seems super useful here as well as in your copySign PR

if(Object.is(`a`, -0)) `result` = "-0.0"
else if(Number.isSafeInteger(`a`)) `result` =  `a`+".0"
else { ...

ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is

we can add the polyfill provided there for IE, if needed

(it may have other uses too, see examples: Object.is([], []); // false) which are hard to do without Object.is

Copy link
Member Author

@ringabout ringabout Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Object.is better than a === 0 && 1 / a < 0?

we can add the polyfill provided there for IE, if needed

It emits more codes if we need to support IE.

I will think of it and look at whether scala.js uses Object.is.

Copy link
Member

@timotheecour timotheecour Dec 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be more efficient; the more code emitted doesn't matter for platforms other than IE as it won't be used

@timotheecour timotheecour mentioned this pull request Dec 29, 2020
@timotheecour
Copy link
Member

@xflywind

VM prints 0.0 as -0.0, I will fix it in the following PR.
Unfortunately, the VM bug is not easy to solve
Uncomment the codes cause the memory of y is polluted

are you using an old nim? I've already fixed this in #16470; it works fine in latest nim devel 95f599c

This test needs to be updated:

when nimvm: discard ## TODO VM print wrong -0.0

since it works

Araq pushed a commit that referenced this pull request Dec 30, 2020
* fix printing negative zero in JS backend

* move tests
@timotheecour timotheecour mentioned this pull request Dec 30, 2020
1 task
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
* fix printing negative zero in JS backend

* move tests
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* fix printing negative zero in JS backend

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

Successfully merging this pull request may close these issues.

3 participants