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

(Towards #2201) Implementation enabling FileContainer's to have information about routines outside of modules. #2575

Merged
merged 12 commits into from
Jun 26, 2024

Conversation

LonelyCat124
Copy link
Collaborator

Likely missing coverage and FAILED psyir/backend/fortran_test.py::test_fw_filecontainer_error1 test fails.

Also waiting on #2566 for fixing some tests before this will be ready.

…the FileContainer Symbol table and fixed the failing test
Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (5bbe862) to head (843b163).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2575   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         351      351           
  Lines       48339    48396   +57     
=======================================
+ Hits        48274    48331   +57     
  Misses         65       65           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LonelyCat124
Copy link
Collaborator Author

This is ready for a look now - I think its a pretty small change overall.

@LonelyCat124
Copy link
Collaborator Author

Changed my mind, there's some missing coverage (which i need to decide if its reachable) and I think I will also fix #2582 here.

@arporter
Copy link
Member

Changed my mind, there's some missing coverage (which i need to decide if its reachable) and I think I will also fix #2582 here.

I think we should fix #2582 by adding Routine._symbol (unless I'm missing something?).

@LonelyCat124
Copy link
Collaborator Author

I think adding Routine._symbol is a bigger change but maybe thats a better place to solve #2582 than doing it effectively twice.

@LonelyCat124
Copy link
Collaborator Author

Ok - I checked this now @sergisiso.
We can't pass a subroutine to either psyir_from_expression or psyir_from_statement, they aren't valid for those entry points so I think we will always have a Container above a subroutine, so I'm going to remove the try/except from here:

try:

routine_symbol = routine.symbol_table.lookup(routine.name)
routine_symbol.datatype = base_type
except KeyError:
 pass

as I think we will always have that routine.name in a container.

@LonelyCat124
Copy link
Collaborator Author

Should now be ready for review.

@arporter
Copy link
Member

Are we best delaying this now until #2596 is done @LonelyCat124 ?

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@LonelyCat124 I would be happy to merge this before finishing #2596 as this is easier and it fixes missing keywords (that nemo compilation has warnings about them) and will allow fixing the function statements in Socrates. See inline comments mostly regarding what happens when there is other things that subroutines in the FileContainer.

@LonelyCat124
Copy link
Collaborator Author

Ready for another look

@LonelyCat124 LonelyCat124 requested a review from sergisiso June 20, 2024 15:06
LonelyCat124 added a commit that referenced this pull request Jun 21, 2024
…s. We need to wait on #2575 to complete this PR for FileContainers as well
@sergisiso
Copy link
Collaborator

@LonelyCat124 I put this back with reviewed with actions to clarify if the last commit after your message that this was ready belongs here, because the same commit is also in your Routine.symbol PR

@LonelyCat124
Copy link
Collaborator Author

@sergisiso Sorry i forgot to check this yesterday. I don't think that commit is on this branch, its just mentioned in the commit message.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

Sorry about the commit confusing @LonelyCat124 , there is just minor things left now.

src/psyclone/psyir/backend/fortran.py Show resolved Hide resolved
src/psyclone/psyir/backend/fortran.py Outdated Show resolved Hide resolved
src/psyclone/psyir/backend/fortran.py Outdated Show resolved Hide resolved
src/psyclone/psyir/frontend/fparser2.py Show resolved Hide resolved
Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@LonelyCat124 All issues have been resolved, and now NEMOv5 is tested again. This is ready to merge. The integration tests were successful but slow probably because the workstation was busy.

@sergisiso sergisiso merged commit f88f535 into master Jun 26, 2024
11 checks passed
@sergisiso sergisiso deleted the 2201_filecontainer_symbol_support branch June 26, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants