-
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
Transformations: ResolveAssociateTransformer re-write to in-place substitution #387
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/387/index.html |
61d511a
to
78f1241
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
=======================================
Coverage 95.52% 95.52%
=======================================
Files 186 186
Lines 38894 38977 +83
=======================================
+ Hits 37152 37234 +82
- Misses 1742 1743 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e21c136
to
4724955
Compare
Instead of finding all symbols and substituting them with their inverse-association, we now simply run over each symbol, find its inverse from the `.scope` and apply in-place. This requires the substitution to be written as a symbol mapper. As an addition feature, we can now do partial resolution in associate bodies, where only a select sub-region of the code has its associated symbols resolved. I've added a test to this extend.
This adds "auto-fixer" behaviour for call.arguments and call.kwarguments, which makes it safe to always iterate them.
4724955
to
4445dd0
Compare
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.
Very neat trick and nicely implemented and tested!
I left a question but that's likely fine, so happy to merge as is.
""" | ||
An :any:`collections.OrderedDict` of associated expressions. | ||
""" | ||
return CaseInsensitiveDict((str(v), k) for k, v in self.associations) |
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.
Can we simply "stringify" v
here, considering that an association can in theory be a rather complex expression?
Note: This PR sits on top of PR #372 and requires this to be merged first.This PR re-writes the
ResolveAssociateTransformer
and switches it to acting entirely in-place. For this, instead of finding all symbols and substituting them with their inverse-association, we now simply run over each symbol, find its inverse from the.scope
and apply in-place. This requires the substitution to be written as a symbol mapper.As an addition feature, we can now do partial resolution in associate bodies, where only a select sub-region of the code has its associated symbols resolved. I've added a test to this extend.
And on top of that I've refactored the imports in the associated test.
Edit: I've also had a to introduce "auto-fixer" behaviour for
CallStatement
nodes and a corresponding test. This now ensure thatcall.arguments
andcall.kwarguments
are always (possibly empty) tuples and can thus safely be iterated. This fixed some fallout in the transpile tests.