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

Builtin math: ^ operator is left-associative #6280

Closed
0x005c opened this issue Nov 3, 2019 · 7 comments
Closed

Builtin math: ^ operator is left-associative #6280

0x005c opened this issue Nov 3, 2019 · 7 comments
Labels
bug good first issue
Milestone

Comments

@0x005c
Copy link
Contributor

@0x005c 0x005c commented Nov 3, 2019

The result of math '3^0.5^2' is currently:

~> math '3^0.5^2'
3

That mean, ^ operator is left-associative.
I think this is a bug because exponentiation operators in many languages are right-associative.

version:

~> fish --version
fish, version 3.0.2-1757-g47c0b5f9
@ridiculousfish ridiculousfish added the bug label Nov 7, 2019
@ridiculousfish ridiculousfish added this to the fish-future milestone Nov 7, 2019
@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Nov 7, 2019

Seems like a fun isolated bug to tackle for anyone interested. Thanks for filing!

@asprazz
Copy link
Contributor

@asprazz asprazz commented Nov 8, 2019

Hie @ridiculousfish, I am really interested. Issue is really interesting and as I am new here I will need a little help. I have built it and I am looking into tinyexpr.cpp but not sure about changes..

@floam
Copy link
Member

@floam floam commented Nov 8, 2019

Easy fix. This is configurable in upstream TinyExpr and that author just likes the left-to-right because that's how spreadsheets do it. I think it will confuse people. Let's change it.

@floam
Copy link
Member

@floam floam commented Nov 8, 2019

If you want to give it a shot @asprazz, go ahead! Look at this code upstream:

https://github.com/codeplea/tinyexpr/blob/ffb0d41b13e5f8d318db95feb071c220c134fe70/tinyexpr.c#L415

See it has a compile time option for TE_POW_FROM_RIGHT? That provides a different version of the factor() function which has a while loop with different logic. Ours needs to be adapted to do the same thing. We've diverged and minimized our code from theirs a bit so there may be some adaptation required, it may be a fun practice to try to apply/translate the same differences you see to our codebase if you like!

@asprazz
Copy link
Contributor

@asprazz asprazz commented Nov 9, 2019

@floam Thank you for clarification. I will be working on this now.. :) 💯

@asprazz
Copy link
Contributor

@asprazz asprazz commented Nov 9, 2019

Hie @floam, Can you please look into my changes ?
I have removed a bunch of code saying => -a^b = -(a^b) as -2^2 = 4 not -4
Not sure about this tho :)
I have found an issue on upstream codeplea/tinyexpr#52

Hey @floam, Thank you for guiding me out. Btw I have created a PR (may be someone will) please review it whenever possible

@ArashPartow
Copy link

@ArashPartow ArashPartow commented Mar 19, 2020

This issue was raised with the author a long time ago:

codeplea/tinyexpr#10 (comment)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug good first issue
Projects
None yet
Development

No branches or pull requests

6 participants