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

Merging function and TemplatedFunction #90

Merged
merged 4 commits into from Aug 18, 2022

Conversation

Oshanath
Copy link
Contributor

@Oshanath Oshanath commented Aug 5, 2022

reference tests pass. I checked and as far as I saw only the extra [] for typevars are added to them.

ctest still dont pass. I will change them next. I will manually generate the ASR for them and paste the result. I checked and there too, the only difference is the extra [] for the typevars.

@certik
Copy link
Contributor

certik commented Aug 5, 2022

Thanks @Oshanath. Here is a TODO:

  • Merge the latest main into this branch. The main now has the code to represent and instantiate templates
  • change the swap test to an add test
  • improve the code to load the new test to ASR, see if we can exactly reuse the existing ASR from main
  • At the instantiation site, simply call the instantiate code, just like LPython does

Ping me if you run into any issues or have any questions.

@Oshanath
Copy link
Contributor Author

Oshanath commented Aug 10, 2022

@certik In the instantiation, I get the types as decl_attributes. I need to convert this into ttypes. For that currently I'm thinking I should use the same code from here. Am I correct?

Shall I refactor that part into a new function so that I can use it? Or is there an easier way?

@certik
Copy link
Contributor

certik commented Aug 10, 2022

Yes, I think you are correct.

That part in AST->ASR in LFortran is quite complex already, so we should figure out how to refactor it. For now just do what you can, refactor it for your needs. We'll come back to it later.

@Oshanath
Copy link
Contributor Author

@certik Users call the instantiation by the name of the template. But currently, the name of the template is not stored anywhere. So I think we should either add a new parameter to Functions to store to which template that function belongs to. Or we should create an ASR node called Template to store its name and the type parameters. I think the latter is the better choice. What do you think?

@certik
Copy link
Contributor

certik commented Aug 10, 2022

I see. I need to think about this, also with relation to restrictions, see Zulip.

@Oshanath
Copy link
Contributor Author

Oshanath commented Aug 12, 2022

@certik I had to change the naming process of the instantiation because the new function name is determined by the user.

x = 5.1
y = 7.2
print*, "The result is ", add_real(x, y)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (abs(add_real(x, y) - 12.3) > 1e-5) error stop

a = 5
b = 9
print*, "The result is ", add_integer(a, b)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (add_integer(x, y) /= 14) error stop

ASR::symbol_t* v_orig = ASRUtils::symbol_get_past_external(v);
 File "$DIR/src/libasr/asr_utils.h", line 43, in ??
if (f->type == ASR::symbolType::ExternalSymbol) {
 Binary file "/lib/x86_64-linux-gnu/libc.so.6", in killpg()
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed now.

tests/tests.toml Outdated
@@ -11,6 +11,11 @@ ast_indent = true
asr_indent = true
ast = true

[[test]]
filename = "../examples/add_m.f90"
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this test from examples into integration_tests.

@certik
Copy link
Contributor

certik commented Aug 17, 2022

This is the latest PR, and it works!!!

So all we need is to just polish this up and merge into master.

@Oshanath
Copy link
Contributor Author

@certik I applied your suggestions, moved the example to integration tests and cleaned up the history.

@certik certik merged commit d0aa865 into lfortran:main Aug 18, 2022
@certik
Copy link
Contributor

certik commented Aug 18, 2022

I went over all commits, everything looks good.

Thanks for finishing this up!

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

2 participants