(towards #3124) Codeblocks with children references #3247
Conversation
… children references
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3247 +/- ##
=======================================
Coverage 99.91% 99.91%
=======================================
Files 375 376 +1
Lines 53442 53531 +89
=======================================
+ Hits 53397 53487 +90
+ Misses 45 44 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@arporter @LonelyCat124 @hiker This is ready for review, it is another step into making the all references expressed in the PSyIR so that tools that expect/operate on References don't need edge cases. |
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
Nice, thanks Sergi.
The only concern I have is that, with this change, we now won't warn if a Symbol incorrectly gets added to the table of a Container. Can we tighten up on that?
I see you ran the integration tests so I won't do that again this time around.
| try: | ||
| symtab = self.scope.symbol_table | ||
| except SymbolError: | ||
| symtab = SymbolTable() |
There was a problem hiding this comment.
Why is this branch necessary? Please add a comment.
| for name in self.get_symbol_names(): | ||
| var_accesses.add_access(Signature(name), AccessType.READWRITE, | ||
| self) | ||
| for child in self.children: |
There was a problem hiding this comment.
Comment please: all symbols accessed within the CodeBlock are captured as Reference nodes and stored as children of the CodeBlock node (or something).
There was a problem hiding this comment.
Added your words
| self.ast_end = None | ||
| # Store the structure of the code block. | ||
| self._structure = structure | ||
| self._insert_representative_references() |
| SymbolTable, SymbolError, UnresolvedInterface) | ||
|
|
||
|
|
||
| class CodeBlock(Statement, DataNode): |
There was a problem hiding this comment.
Please update the class docstring to record that it does now have children.
There was a problem hiding this comment.
(I've checked the description of CodeBlocks in the documentation and it doesn't require any updating.)
| continue | ||
| result.append(node.string) | ||
| # Precision on literals requires special attention since they are just | ||
| # stored in the tree as str (fparser/#456). |
There was a problem hiding this comment.
Not yours but L263 is not covered by the associated test file. Perhaps one for @LonelyCat124 to fix while he's looking at directives?
| # s symbol should not be in my_mod | ||
| with pytest.raises(KeyError): | ||
| psyir.children[0].symbol_table.lookup("s") | ||
| # FIXME: explain |
| ''' | ||
| fake_parent, fparser2spec = process_where( | ||
| "WHERE (ptsu(myfunc(), :, :) /= 0._wp)\n" | ||
| "WHERE (ptsu(unres, :, :) /= 0._wp)\n" |
There was a problem hiding this comment.
What was the reason for this change?
| assert "myifblock" not in sym_names | ||
| assert "this_is_true" in sym_names | ||
| assert "that_is_true" in sym_names | ||
| refs = block.walk(Reference) |
There was a problem hiding this comment.
Please could you also check that these refs are immediate children of the CodeBlock (as this is new).
Your use of 'virtual refs' to describe them did make me wonder whether we should have a new VirtualReference subclass of Reference but I'm not sure that buys us anything other than making it clear that these aren't "real" references.
| ''' Tests the infer_sharing_attributes() method when some of the loops have | ||
| Codeblocks inside it. We check that the infer_sharing attribute analysis | ||
| succeed by assuming worst case. | ||
| potentially hidden references (e.g. inside codeblocks or loop varialbes). |
| loop = psyir.walk(Loop)[loop_idx] | ||
| # Make sure that the write statements inside the loop are CodeBlocks, | ||
| # otherwise we need a new test example | ||
| assert loop.has_descendant(nodes.CodeBlock) |
There was a problem hiding this comment.
We still need something like this check - probably has to be a walk now.
No description provided.