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

chore: Make pow use early return #1706

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Feb 28, 2023

This pr refactors pow to use early return.

This also adds a small optimization to Rational**Int Cases.

stdlib/runtime/numbers.gr Outdated Show resolved Hide resolved
@spotandjake
Copy link
Member Author

I went with the approach of just stealing the code out from rationalDenominator and rationalNumerator but I am not 100% sure I put the incRef call in the correct place.

@ospencer
Copy link
Member

ospencer commented Mar 1, 2023

Could you just call rationalDenominator and rationalNumerator directly?

@spotandjake
Copy link
Member Author

Could you just call rationalDenominator and rationalNumerator directly?

I would have to do what alex was saying of WasmI32.toGrain(WasmI32.fromGrain(base)): Rational given it wants a Rational wouldnt I. and then that requires a Memory.incRef call right? would the incRef?

@ospencer
Copy link
Member

ospencer commented Mar 2, 2023

Yep, you'd need to do that. The incref just goes in the middle.

@spotandjake
Copy link
Member Author

Also is this breaking given 1/2^3 will no longer give 0.125 but will give 1/8?

@ospencer
Copy link
Member

ospencer commented Mar 2, 2023

Yeah, certainly that too. It's probably worth it to separate them into two PRs, one that does early return, and the other that changes the squaring of rationals. Do you want to let this PR be the early return one and you can open another after with the behavior change?

@spotandjake
Copy link
Member Author

👍

@spotandjake
Copy link
Member Author

I reverted those commits after this gets merged I will add that on a separate pr.

@ospencer ospencer added this pull request to the merge queue Mar 2, 2023
Merged via the queue into grain-lang:main with commit dc1b677 Mar 2, 2023
av8ta pushed a commit to av8ta/grain that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants