-
Notifications
You must be signed in to change notification settings - Fork 12
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
ProgramUnit.resolve_typebound_var: raise error if top-level parent is not declared #325
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/325/index.html |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
+ Coverage 95.14% 95.19% +0.04%
==========================================
Files 167 168 +1
Lines 35374 35717 +343
==========================================
+ Hits 33657 34001 +344
+ Misses 1717 1716 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
loki/tests/test_subroutine.py
Outdated
""".strip() | ||
|
||
source = Sourcefile.from_source(fcode, frontend=frontend, xmods=[tmp_path]) | ||
# routine = Subroutine.from_source(fcode_routine, frontend=frontend, xmods=[tmp_path], definitions=source.definitions) #source['some_routine'] |
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.
Debug leftover?
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.
Ooops.
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.
Ok, this looks great and is indeed very useful and makes things safer. I've left one question on the test, because I don;t quite understand when/what is invalid, and what is not. There's also a rogue commented line still in test - but once those are addressed, this is GTG, I think.
loki/tests/test_subroutine.py
Outdated
# This does not throw an error as the use-case of incomplete type definitions | ||
# may well require working with incomplete type definitions | ||
tt_invalid_val = routine.resolve_typebound_var('tt%invalid%val') | ||
assert tt_invalid_val == 'tt%invalid%val' |
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'm not sure I understand the reasoning here. The type of tt
is known so it can be safely inferred that tt%invalid
is in deed invalid. If the type of tt
were deferred, I'd agree, but it is attached in this case, no?
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.
Right, I see your point. I was thinking of situations where we don't have the type definition and overlooked the fact that we actually have the complete definition here. Let me see if I can quickly add an error in this case without harming the situation where we don't have the full type definition.
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.
Looks great. Many thanks for addressing this.
…typedef is available
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.
Perfect! Looks great now, GTG from me.
This fixes #307.
Piggy-backed is a small fix with test for missing enrichment between modules within the same source file, reported in #161.