-
Notifications
You must be signed in to change notification settings - Fork 170
Draft: Adding generic functions to LPython #625
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
Conversation
Can you please resolve conflicts and add tests? |
About the conflict, I'm not sure what About the tests, currently there is no code generation for type variables, so what kind of test should I make? Also I am not familiar with the steps to add tests. |
@ansharlubis see if you can rebase this on top of the latest main. Simply at the end, your additions should look the same, just on top of the latest main. Start with that. Then we can talk about tests. If you get stuck, please let me know right away, and I'll help. |
src/libasr/ASR.asdl
Outdated
@@ -317,6 +317,10 @@ ttype | |||
| Dict(ttype key_type, ttype value_type) | |||
| Pointer(ttype type) | |||
| CPtr() | |||
| TypeVar(string var) | |||
|
|||
boolop = And | Or | Xor | NEqv | Eqv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably remove this, we don't need it for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need the | TypeVar(String var)
for handling type variable declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need TypeVar
, but you do not need the extra boolop
.
Now when it is merged with the latest main, the next step is to add a test for this. I think this takes Python code and converts it to ASR? If so, then add a test into the |
How do I define the reference for the test? |
You run |
I seem to run into problem with
What is the purpose of |
In there the purpose is to declare |
TODO:
|
@@ -0,0 +1,57 @@ | |||
Traceback (most recent call last): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be removed.
tests/tests.toml
Outdated
[[test]] | ||
filename = "../integration_tests/generics_01.py" | ||
asr = true | ||
llvm = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to test llvm, since we are not modifying anything in the backend. Rather, let's add an integration test, to ensure it builds and runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay got it. I've already added the integration test.
tests/tests.toml
Outdated
[[test]] | ||
filename = "../integration_tests/generics_array_01.py" | ||
asr = true | ||
llvm = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, let's add an integration test and remove llvm here.
Keep testing asr, that is good.
What is the status of this and #831? Let's get it finished so that we can merge. |
We do not need to work on this one, since we are merging #831, so I'll close this one. |
No description provided.