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

Upgrade exponentiation to unified operator #7153

Merged
merged 11 commits into from
Mar 22, 2025

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Nov 9, 2024

Also make its output to use ES7 exponentiation (**) operator.

** is more concise, faster than Math.pow(),
works well with all numeric types include bigint.

We were already using it for bigint, now for int and float too.

@cometkim cometkim mentioned this pull request Nov 9, 2024
@cometkim
Copy link
Member Author

cometkim commented Nov 10, 2024

I think (x ** y | 0) is not very same as the current behavior of %mul, which uses Math.imul() for ints

The problem is that to match the semantics we need to perform the modulo operation ourselves, which is quite expensive at runtime.

@cometkim
Copy link
Member Author

Well, I guess I'll have to propose new int semantics to fix it, but that would be a pretty breaking change.

It seems very weird now that all other operations use int32 coercion, but only multiplication has a different behavior.

@cristianoc
Copy link
Collaborator

I think (x ** y | 0) is not very same as the current behavior of %mul, which uses Math.imul() for ints

The problem is that to match the semantics we need to perform the modulo operation ourselves, which is quite expensive at runtime.

What is the difference, in short?

@cristianoc
Copy link
Collaborator

Oh you mean (x * y) | 0 vs Math.imul(x, y)?

@cometkim
Copy link
Member Author

(discussing in rescript-lang/rfcs#1)

Also make its output to use ES7 exponentiation (`**`) operator.

`**` is more concise, faster than `Math.pow()`,
works well with all numeric types include bigint.

We were already using it for bigint, now for int and float too.
@cometkim
Copy link
Member Author

The problem I mentioned is this doesn't meet the following requirement:

The exponentiate(x, y) must produce the same result as multiply(x) accumulated y times.

But I think we'll end up fixing the %mulint behavior, so I'm merging this implementation.

@cometkim cometkim merged commit 9c6856d into rescript-lang:master Mar 22, 2025
20 checks passed
@cometkim cometkim deleted the exp-operator branch March 22, 2025 16:55
@cometkim
Copy link
Member Author

The problem I mentioned is this doesn't meet the following requirement:

The exponentiate(x, y) must produce the same result as multiply(x) accumulated y times.

But I think we'll end up fixing the %mulint behavior, so I'm merging this implementation.

PR opened: #7358

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.

2 participants