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

Modulo(3rd argument) added to pow #876

Merged
merged 2 commits into from Aug 4, 2022

Conversation

Madhav2310
Copy link
Contributor

No description provided.

@czgdp1807
Copy link
Collaborator

czgdp1807 commented Aug 3, 2022

I think you haven't updated reference tests. Please run, python run_tests.py -u, commit and then push.

@czgdp1807
Copy link
Collaborator

@Smit-create I just met @Madhav2310. He said that he is facing some issues with overload operator (he received an error saying only 2 arguments are accepted by 3 provided). Can you fetch his branch and try to run the tests on your machine locally? Let me know if you face the same issue.

@Smit-create
Copy link
Collaborator

Yes, I get the same error as:

semantic error: Arguments do not match for any generic procedure, pow
  --> expr.py:22:16
   |
22 |     assert abs(pow(i,j,k) - 4) < eps
   |                ^^^^^^^^^^ 

Overload is working fine for the following example:

@overload
def a(x: i32) -> i32:
    return x+1

@overload
def a(x: i32, y:i32) -> i32:
    return x+y

def f():
    print(a(2), a(5, 6))

f()

@czgdp1807
Copy link
Collaborator

czgdp1807 commented Aug 3, 2022

Can you try for three argument example?

@Smit-create
Copy link
Collaborator

Can you try for three argument example?

Works fine. Even this diff also works:

diff --git a/src/runtime/math.py b/src/runtime/math.py
index ee0bbed29..81a991713 100644
--- a/src/runtime/math.py
+++ b/src/runtime/math.py
@@ -615,3 +615,9 @@ def remainder(x: f64, y: f64) -> f64:
     if x - y*q > y*(q + 1) - x:
         return x - y*(q + 1)
     return x - y*q
+
+def f():
+    r: i32
+    r = pow(102, 3, 121)
+    print(r)
+f()

It prints 38

@czgdp1807
Copy link
Collaborator

Well, then last option to reproduce @Madhav2310 's error is to just fetch his branch and run the tests locally with his code changes. @namannimmo10 and @Smit-create Can you please do that?

@Smit-create
Copy link
Collaborator

Ahhh, I got that error because I didn't import pow from math.py. The error which Madhav is getting is due to CPython:

TypeError: pow expected 2 arguments, got 3

@Smit-create
Copy link
Collaborator

Smit-create commented Aug 3, 2022

CPython's math doesn't have 3 args. See: https://docs.python.org/3/library/math.html#math.pow

Python 3.10.2 | packaged by conda-forge | (main, Feb  1 2022, 19:30:18) [Clang 11.1.0 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from math import pow
>>> pow(23, 24, 13)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: pow expected 2 arguments, got 3

@czgdp1807
Copy link
Collaborator

I see. The builtin pow has three arguments not the math.pow. @Madhav2310 used 3 arguments with math.pow. I think he should add a separate test for builtin pow. Anyways, @Smit-create did this so it should be fixed now.

Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@Smit-create Smit-create merged commit bcf8b25 into lcompilers:main Aug 4, 2022
Copy link
Collaborator

@namannimmo10 namannimmo10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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.

None yet

4 participants