(closes #3265 and #1418) Improve parsing Fortran Derived Types with unsupported components #3267
Conversation
sergisiso
temporarily deployed
to
integration
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3267 +/- ##
=======================================
Coverage 99.95% 99.95%
=======================================
Files 377 377
Lines 53667 53707 +40
=======================================
+ Hits 53645 53685 +40
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…en the declaration is found
…rived_type_components
|
@arporter @LonelyCat124 This is ready for review. It fixes a couple of issue we have parsing derived types. |
sergisiso
changed the title
sergisiso
changed the title
sergisiso
changed the title
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
Good job Sergi. Just need a few comments and clarification of a couple of things. Plus testing for CONTAINS.
| ''' | ||
| if not isinstance(symbol, Symbol): | ||
| raise TypeError(f"Expected a Symbol but got '{type(symbol).__name__}'") | ||
| if not isinstance(symbol, (Symbol, StructureType.ComponentType)): |
There was a problem hiding this comment.
Pls update the :raises: part of the docstring too.
| this contains the single entry "*". | ||
| :param preceding_comments: list of comments preceding the declaration. | ||
| :raise SymbolError: a symbol with the same name already in the given |
| = self._comments_list_to_string( | ||
| preceding_comments) | ||
| preceding_comments = [] | ||
| self._last_psyir_parsed_and_span\ |
There was a problem hiding this comment.
Can you remind me/add comment what this is for?
|
|
||
| preceding_comments = [] | ||
| for child in decl.children: | ||
| for child in decl.children[1:]: |
There was a problem hiding this comment.
Comment on the use of 1: please.
| raise NotImplementedError( | ||
| "Derived-type definition contains unsupported attributes.") | ||
|
|
||
| # We don't yet support derived-type definitions with a CONTAINS |
There was a problem hiding this comment.
Note to myself to remember to check that the appropriate test is updated.
There was a problem hiding this comment.
It already exists in src/psyclone/tests/psyir/frontend/fparser2_derived_type_test.py::test_derived_type_contains
| :param symbol_table: the symbol table in which to add new symbols. | ||
| :param visibility_map: mapping of symbol names to explicit | ||
| visibilities. | ||
| :type visibility_map: dict with str keys and values of type |
There was a problem hiding this comment.
Pls mv this to be a type-hint.
| :param scope: PSyIR node in which to insert the symbols found. | ||
| :type scope: :py:class:`psyclone.psyir.nodes.ScopingNode` | ||
| :param scope: the PSyIR location of the declaration. | ||
| :param symbol_table: the symbol table in which to add new symbols. |
There was a problem hiding this comment.
I was going to ask before but presumably you've added this because sometimes we don't want to use the scope.symbol_table?
There was a problem hiding this comment.
yes, sometimes we populate a new symbol_table, but we still provide the original scope so that variables on the dimensions/initialisation expressions can be found.
I have to admit this is a bit concerning to me but the _process_decl already had this signature, I am just passing it down, so it behaves the same as the _process_decl implies it does.
| self._process_decl_or_create_unsupported( | ||
| parent, local_table, component, | ||
| preceding_comments=preceding_comments) | ||
| preceding_comments = [] |
There was a problem hiding this comment.
I think process_decl_or_create_unsupported resets preceding_comments?
There was a problem hiding this comment.
fparser2_comment_test.py has some failures if I delete this line, so maybe not all exits of the method clears it?
| self._process_decl_or_create_unsupported( | ||
| parent, parent.symbol_table, node, visibility_map, | ||
| statics_list, preceding_comments) | ||
| preceding_comments = [] |
There was a problem hiding this comment.
Again, could you check whether this reset of preceding_comments is needed?
There was a problem hiding this comment.
It is needed to pass the tests.
| UnsupportedFortranType) | ||
|
|
||
|
|
||
| def test_type_with_outside_reference(f2008_parser): |
There was a problem hiding this comment.
Please could you also add (or check for existing) test of a structure with a CONTAINS.
There was a problem hiding this comment.
As mentioned we have: test_derived_type_contains
|
@arporter This is ready for another review. I had to re-introduce the exclusion for fldread but it turns out this is because the issue was not due to derived type issue but due to a lhs-rhs dependency in one of those references, this is explored in an existing issue which I have updated with this case. The good news is that all obs_* files, that previously we had to blanked exclude, now work. Also the performance is back to where it was before #3266, meaning that we know need to check the datatype before extending arrays, but also that we manage to parse and understand all components that matter in order to do it. |
No description provided.