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

pass "nopass" for class procedure having nopass and handle optional arguments for ClassProcedure with/without nopass #3711

Merged
merged 12 commits into from
Mar 28, 2024

Conversation

gxyd
Copy link
Contributor

@gxyd gxyd commented Mar 26, 2024

pass "nopass" presence along to be used by CallBody

I've written the (only) test case with just --show-asr, as the --show-llvm will still raise an error because of a problem in transform_optional_argument_function pass (for details, see: #3632)

@gxyd gxyd force-pushed the module_function_nopass branch 3 times, most recently from c392342 to f307305 Compare March 27, 2024 06:59
@gxyd gxyd changed the title fix: pass "nopass" for module function fix: pass "nopass" for class procedure having nopass Mar 27, 2024
@Pranavchiku
Copy link
Contributor

Can you try updating test references and see if CI passes.

Copy link
Contributor

@Pranavchiku Pranavchiku 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, let's get CI working, dropped comments to improve test.

@Pranavchiku Pranavchiku marked this pull request as draft March 27, 2024 09:05
@gxyd gxyd marked this pull request as ready for review March 27, 2024 09:31
@gxyd
Copy link
Contributor Author

gxyd commented Mar 27, 2024

Can you try updating test references and see if CI passes.

I think it was a failure similar to the one mentioned in this issue: #3707. I've pushed a commit, hopefully the tests pass now.

@gxyd
Copy link
Contributor Author

gxyd commented Mar 27, 2024

If you do like the changes, and accept it, I would request for it to be a squash the commits when merging.

@Pranavchiku
Copy link
Contributor

Pranavchiku commented Mar 27, 2024

I am just not sure of these nopass stuff, so I'll prefer to wait for one more approval before merging :)
Thanks for your work!

@gxyd
Copy link
Contributor Author

gxyd commented Mar 27, 2024

I am just not sure of these nopass stuff, so I'll prefer to wait for one more approval before merging :)

Sounds good.

@certik certik marked this pull request as draft March 27, 2024 16:32
@@ -1476,3 +1476,4 @@ RUN(NAME types_real_to_complex_cast LABELS gfortran llvm)
RUN(NAME types_real_array_to_complex_array_cast LABELS gfortran llvm)

RUN(NAME lbound_01 LABELS gfortran llvm)
RUN(NAME module_function_with_nopass gfortran)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now tested with GFortran.

@gxyd gxyd marked this pull request as ready for review March 27, 2024 16:56
@gxyd
Copy link
Contributor Author

gxyd commented Mar 28, 2024

@certik , this is now ready for a review. I've made more changes after your last review. We should now be able to compile the test case completely (not just till ASR), and it's tested as well (I've used llvm2 label for it).

The issue #3632 still remains relevant (when nopass isn't passed to the class procedure), which depends on the changes introduced in this MR. Let me know what you think.

Update: The issue #3632 is also now handled in this PR.

@gxyd gxyd changed the title fix: pass "nopass" for class procedure having nopass fix: pass "nopass" for class procedure having nopass and handle optional arguments for ClassProcedure with/without nopass Mar 28, 2024
@gxyd gxyd changed the title fix: pass "nopass" for class procedure having nopass and handle optional arguments for ClassProcedure with/without nopass pass "nopass" for class procedure having nopass and handle optional arguments for ClassProcedure with/without nopass Mar 28, 2024
@gxyd
Copy link
Contributor Author

gxyd commented Mar 28, 2024

This PR now also fixes #3632

@gxyd
Copy link
Contributor Author

gxyd commented Mar 28, 2024

@certik , could you help me understand why the two CI checks are failing?

@Pranavchiku
Copy link
Contributor

CI failure:

[ 19%] Building Fortran object CMakeFiles/print_03_FAST.dir/print_03.f90.o
[ 23%] Building Fortran object CMakeFiles/print_05_FAST.dir/print_05.f90.o
make[2]: *** [CMakeFiles/module_function_with_nopass_FAST.dir/module_function_with_nopass.f90.o] Segmentation fault: 11
[ 26%] Linking Fortran executable error_stop_02_FAST
make[1]: *** [CMakeFiles/module_function_with_nopass_FAST.dir/all] Error 2
[ 30%] Linking Fortran executable error_stop_01_FAST
make[1]: *** Waiting for unfinished jobs....
[ 34%] Linking Fortran executable print_01_FAST
[ 38%] Linking Fortran executable stop_01_FAST
[ 42%] Linking Fortran executable print_03_FAST
make: *** [all] Error 2

@gxyd
Copy link
Contributor Author

gxyd commented Mar 28, 2024

Ah, I see. I did see the error ofcourse, but I didn't know that you can simply run that locally by lfortran integration_tests/module_function_with_nopass.f90 --fast, i.e. by adding --fast in the command. I don't much about --fast, I only see: (Best performance (disable strict standard compliance), which doesn't make it much clear for me honestly.

@certik
Copy link
Contributor

certik commented Mar 28, 2024

Related #3733.

tests/tests.toml Outdated Show resolved Hide resolved
@certik
Copy link
Contributor

certik commented Mar 28, 2024

The CI failure seems unrelated: #3736.

@gxyd
Copy link
Contributor Author

gxyd commented Mar 28, 2024

Yes, something along the lines of #3619

@gxyd
Copy link
Contributor Author

gxyd commented Mar 28, 2024

Can we squash the commits when merging? (If you also think that makes more sense)

@gxyd
Copy link
Contributor Author

gxyd commented Mar 28, 2024

As otherwise, they would introduce un-necessary reference files in our commit history.

@certik certik enabled auto-merge (squash) March 28, 2024 18:44
@certik
Copy link
Contributor

certik commented Mar 28, 2024

Tests pass on a rerun. I rebased and auto-squash-merged.

@certik certik merged commit 81308c6 into lfortran:main Mar 28, 2024
21 checks passed
@gxyd gxyd deleted the module_function_nopass branch March 29, 2024 05:30
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

3 participants