(closes #2642) Support structure type extends and contains - #3563
(closes #2642) Support structure type extends and contains#3563sergisiso wants to merge 13 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3563 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 397 397
Lines 55775 56023 +248
==========================================
+ Hits 55775 56023 +248 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@arporter @LonelyCat124 This is ready to review, it is a necesarry step to read the metadata using psyir. Wihtout this the embedded metadata ends up in an UnsupportedFortranType, and we still would need to parse it as a string. It seems that Rupert started this path of parsing the metadata from the UnsupportedType as a string, at the moment I convert the new datatype back to an string and then pass it to existing metadata parsing to keep coverage of the existing tests (which feels odd). The previous PR adding this this feature #2644 also hit the same problem. However, I have a follow up PR that converts the metadata parsing to take proper PSyIR (removing the need of fparser1 and string metadata parsing) and the code that lose coverage can be removed altoghether. But the resulting PR was huge. In fact I cut this branch to make the whole review more manegable. |
LonelyCat124
left a comment
LonelyCat124
left a comment
There was a problem hiding this comment.
@sergisiso From an implementation perspective I think I'm mostly ok with this (if I understood it correctly). However, there are some areas lacking comments, some missing typehints (some of which are old missing but you've modified the function) and some typehints which I believe are wrong. See comments inline.
| # replace this | ||
| declaration = FortranWriter().gen_typedecl( | ||
| symbol, include_visibility=False) | ||
| # Preserve the spelling |
There was a problem hiding this comment.
| # Preserve the spelling | |
| # Preserve the casing |
?
| str(self.name), UnsupportedFortranType(self.fortran_string())) | ||
|
|
||
| @staticmethod | ||
| def create_from_psyir(symbol): |
There was a problem hiding this comment.
typehints here please.
| '''Create a new instance of GOceanKernelMetadata populated with | ||
| metadata from a kernel in language-level PSyIR. | ||
|
|
||
| :param symbol: the symbol in which the metadata is stored \ |
There was a problem hiding this comment.
Remove any remaining \ from the docstring.
| f"'{coarse_function_space}'")) | ||
|
|
||
| @staticmethod | ||
| def create_from_psyir(symbol): |
| '''Create a new instance of LFRicKernelMetadata populated with | ||
| metadata from a kernel in language-level PSyIR. | ||
|
|
||
| :param symbol: the symbol in which the metadata is stored \ |
| f"The visibility of a procedure component of a StructureType " | ||
| f"must be an instance of 'Symbol.Visibility' but got " | ||
| f"'{type(visibility).__name__}'") | ||
| if (initial_value is not None and |
There was a problem hiding this comment.
Can we just do if (initial_value and ...?
|
|
||
| return True | ||
|
|
||
| def replace_symbols_using(self, table_or_symbol): |
| preceding_comment=component.preceding_comment, | ||
| inline_comment=component.inline_comment) | ||
|
|
||
| for component in list(self.procedure_components.values()): |
There was a problem hiding this comment.
Brief comment explaining the block, e.g.
# Add the procedure components and setup the intial values of the components where appropriate
Or similar.
| component.name, new_type, component.visibility, | ||
| initial_value, component.preceding_comment, | ||
| component.inline_comment) | ||
|
|
There was a problem hiding this comment.
# Maintain the inheritance if necessary
or similar.
| assert "Expected a DataTypeSymbol but found a str." in str(info.value) | ||
| metadata = GOceanKernelMetadata.create_from_psyir(symbol) | ||
| assert METADATA in metadata.fortran_string() | ||
| assert METADATA.upper() in metadata.fortran_string().upper() |
There was a problem hiding this comment.
Why does the check require the upper case change now? If there's a specific reason then please add a comment so its clear as to why (since PSyclone should be consistent in its output)
3 participants
No description provided.