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

Adding type parameters to functions without return values #957

Closed
wants to merge 9 commits into from

Conversation

ansharlubis
Copy link
Collaborator

No description provided.

@certik
Copy link
Contributor

certik commented Aug 14, 2022

I think that this is good. Should we add some tests now or later?

@ansharlubis
Copy link
Collaborator Author

Do you mean tests for subroutines? I can do that right away.

@ansharlubis
Copy link
Collaborator Author

ansharlubis commented Aug 15, 2022

Turned out there really were problems with subroutines, so I made some more modifications and added a swapping example.

@@ -6,5 +6,3 @@ def f(x: T, y: T) -> T:
return x + y

print(f(1,2))
print(f("a","b"))
print(f("c","d"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's return these back.

 File "$DIR/src/libasr/pass/instantiate_template.cpp", line 327, in LFortran::pass_instantiate_generic_function(Allocator&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, LFortran::ASR::ttype_t*, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, LFortran::ASR::ttype_t*> > >, LFortran::SymbolTable*, int, LFortran::ASR::Function_t&)
ASR::asr_t *new_function = tf.instantiate_Function(func);
 File "$DIR/src/libasr/pass/instantiate_template.cpp", line 61, in LFortran::FunctionInstantiator::instantiate_Function(LFortran::ASR::Function_t&)
(ASR::down_cast<ASR::Var_t>(x.m_return_var))->m_v);
Copy link
Contributor

Choose a reason for hiding this comment

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

This file can be removed.

@@ -796,6 +879,7 @@ class CommonVisitor : public AST::BaseVisitor<Derived> {
visit_expr_list_with_cast(func->m_args, func->n_args, args_new, args);
return ASR::make_SubroutineCall_t(al, loc, stemp,
s_generic, args_new.p, args_new.size(), nullptr);
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

If this section is not needed anymore, should it be removed?

@certik
Copy link
Contributor

certik commented Aug 17, 2022

Otherwise this looks good in general.

@ansharlubis
Copy link
Collaborator Author

I've cleaned up the rest, if this passes the check I'll merge and clean the git history as well.

@@ -7,4 +7,4 @@ def f(x: T, y: T) -> T:

print(f(1,2))
print(f("a","b"))
print(f("c","d"))
print(f("c","d"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a small thing, but if the only change is just a white space, let's not do it in this PR, you can fix that by:

git checkout main integration_tests/generics_01.py

Then update tests too, it will remove the change in the reference result for this.

@@ -761,41 +775,27 @@ class CommonVisitor : public AST::BaseVisitor<Derived> {
return ASR::make_Assignment_t(al, loc, variable_var, ASRUtils::EXPR(func_call_asr), nullptr);
} else {
return func_call_asr;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove whitespace.

@certik
Copy link
Contributor

certik commented Aug 18, 2022

If you do a final rebase / polishing of the git history, please ping me one more time before the merge, so that I can review everything.

@certik
Copy link
Contributor

certik commented Aug 18, 2022

I think that looks good. If tests pass, you can go ahead and polish the commits.

@ansharlubis
Copy link
Collaborator Author

Please check PR #989 where I've cleaned up the history.

@ansharlubis ansharlubis deleted the subroutine-template branch August 24, 2022 07:59
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.

2 participants