-
Notifications
You must be signed in to change notification settings - Fork 170
Add overload support in lpython #187
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
I think the symbol table visitor can do all of this, correct? Or do we need to handle this in the body visitor also? If just symbol table visitor, then I think what you can do is to start the ASR GenericProcedure node, and just keep adding to it. I think. You can see how this is done in LFortran, I can't remember right now. Otherwise using a temporary structure like you have done in this PR also works. |
Will have a look how that is done. |
On the current diff, I get: from ltypes import overload, i32
@overload
def foo(a: i32, b: i32) -> i32:
return a*b
@overload
def foo(a: i32) -> i32:
return a**2 ASR: (TranslationUnit
(SymbolTable
1
{
__lpython_overloaded_0__foo:
(Function
(SymbolTable
2
{
_lpython_return_variable:
(Variable
2
_lpython_return_variable
ReturnVar () ()
Default
(Integer 4 [])
Source
Public
Required .false.),
a:
(Variable
2
a
In () ()
Default
(Integer 4 [])
Source
Public
Required .false.),
b:
(Variable
2
b
In () ()
Default
(Integer 4 [])
Source
Public
Required .false.)
})
__lpython_overloaded_0__foo [
(Var 2 a)
(Var 2 b)] [
(=
(Var 2 _lpython_return_variable)
(BinOp
(Var 2 a)
Mul
(Var 2 b)
(Integer 4 []) () ()) ())
(Return)]
(Var 2 _lpython_return_variable)
Source
Public
Implementation ()),
__lpython_overloaded_1__foo:
(Function
(SymbolTable
3
{
_lpython_return_variable:
(Variable
3
_lpython_return_variable
ReturnVar () ()
Default
(Integer 4 [])
Source
Public
Required .false.),
a:
(Variable
3
a
In () ()
Default
(Integer 4 [])
Source
Public
Required .false.)
})
__lpython_overloaded_1__foo [
(Var 3 a)] [
(=
(Var 3 _lpython_return_variable)
(BinOp
(Var 3 a)
Pow (ConstantInteger 2
(Integer 4 []))
(Integer 4 []) () ()) ())
(Return)]
(Var 3 _lpython_return_variable)
Source
Public
Implementation ()),
main_program:
(Program
(SymbolTable
4
{
})
main_program [] [])
})
[]) |
We will now need to add support for overload in |
The ASR example you posted is almost ok, but it needs to also add |
I thought since we are changing the name of the function/subroutine, it might not be that important to use |
What is your idea/design of making this work without a GenericProcedure in ASR? By storing the list of overloaded procedures in How would that work with independent compilation of modules to "mod" files, and then loading them in a new The idea that we have followed so far is to keep ASR self-contained, so that it contains all the information that you might need later. It seems in this PR we are not storing all the information for generic subroutines in ASR, but keeping it outside of it. |
Yeah, that makes sense to me now. I'll try it using |
The PR looks all good to go once the tests passes. Just need to figure out how to get rid of the global variable |
@@ -139,6 +139,8 @@ ASR::Module_t* load_module(Allocator &al, SymbolTable *symtab, | |||
return mod2; | |||
} | |||
|
|||
std::map<int, ASR::symbol_t*> ast_overload; |
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.
The last part to do is to pass this via arguments to the constructor of the visitor classes and have it as a local variable in python_ast_to_asr
. Then it will not clash when modules are imported recursively.
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.
Then it will not clash when modules are imported recursively.
That's fine, but I guess it won't be a big issue since we are storing the memory reference of each function which will indeed be unique.
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.
Only if you get lucky. When you load multiple modules, the visitor gets created, then destroyed. I think the nodes are allocated using our linear allocator, so I guess they would still be unique. But then when you compile multiple stuff from the main driver and happen to create a new allocator, then it will possibly collide. So we should not rely on that and have no global state.
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.
How about using it as a class variable and clearing it completely when a symbol table visitor is instantiated?
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.
That will erase it when you import other modules recursively, wouldn't it?
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.
Ahh, yes! One probable approach can be to add an optional arguement in .FunctionDef
to store the index where it will be present in GenericNode
Otherwise I think this looks good. |
Let me know if you need any help with implementing the passing of the |
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 moved the ast_overload
into a local variable, so it should be good to go in now. Thanks @Smit-create !
Thanks for updating the PR and merging!! |
Fixes #141.