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

Consider merging LiteralInfinityExpression into LiteralNumericExpression #71

Open
RReverser opened this issue Feb 5, 2019 · 3 comments

Comments

@RReverser
Copy link
Contributor

As discussed on Gitter, we might want to do this to simplify handling and provide optimised representations (like binast/binjs-ref#239) more easily for both kinds of numbers.

Some points from the discussion:

  • double can store infinity values just fine in any IEEE.754 compatible representation, so this shouldn't be an issue, although one voiced concern is that WebIDL defines double as finite floating 64-bit numbers, but this likely shouldn't be a problem due to how we use it.
  • Another concern is having to provide a specialised codegen for infinity values in LiteralNumericExpression that would produce something like 1e111111111... (see https://bugzilla.mozilla.org/show_bug.cgi?id=1524302#c2), but implementors would need to do the same for LiteralInfinityExpression as well, so it's only a matter of place where to put the branching.

cc @syg @Yoric @arai-a

@michaelficarra
Copy link
Member

As far as codegen, 2e308 is the common "canonical" representation of Infinity for doubles. So that's not really a problem. But you should consider whether most usage will care to differentiate between finite and infinite numbers.

Another consideration you may have is JSON representation, which is the original motivator for the Shift AST to have these two nodes separated.

@RReverser
Copy link
Contributor Author

But you should consider whether most usage will care to differentiate between finite and infinite numbers.

Aside from stringification, currently other cases (like the one mentioned above) seem to actually benefit from having these represented in the same way.

Another consideration you may have is JSON representation

But this is a good point. While (I believe) JSON is a second-class citizen for BinaryAST and is mostly intended for debugging purposes, this would have to be solved somehow if the change is adopted.

@Yoric
Copy link
Collaborator

Yoric commented Feb 8, 2019

While (I believe) JSON is a second-class citizen for BinaryAST and is mostly intended for debugging purposes, this would have to be solved somehow if the change is adopted.

At the moment, that's true, because we used a strongly-typed internal representation, but that's no written in stone.

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

No branches or pull requests

3 participants