proposal-binary-ast icon indicating copy to clipboard operation
proposal-binary-ast copied to clipboard

Consider merging LiteralInfinityExpression into LiteralNumericExpression

Open RReverser opened this issue 6 years ago • 3 comments

As discussed on Gitter, we might want to do this to simplify handling and provide optimised representations (like https://github.com/binast/binjs-ref/issues/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

RReverser avatar Feb 05 '19 12:02 RReverser

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.

michaelficarra avatar Feb 05 '19 17:02 michaelficarra

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.

RReverser avatar Feb 05 '19 18:02 RReverser

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.

Yoric avatar Feb 08 '19 12:02 Yoric