-
Notifications
You must be signed in to change notification settings - Fork 30
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
(Closes #2125) Fix bugs in SymbolTable deep copy and InlineTrans when array bounds reference formal arg #2633
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2633 +/- ##
==========================================
- Coverage 99.86% 99.85% -0.02%
==========================================
Files 352 352
Lines 48512 47308 -1204
==========================================
- Hits 48447 47239 -1208
- Misses 65 69 +4 ☔ View full report in Codecov by Sentry. |
This is ready for a first look now. One for @sergisiso, @hiker or @LonelyCat124. I've triggered the integration tests. |
(I am wondering whether having the "force" option to InlineTrans mean "ignore CodeBlocks" is a good idea: perhaps it should be more specific such as "permit-codeblocks"?) |
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.
@arporter I agree this needs to be done, as now we carelessly leave symbols pointing behind. But there are more symbols that the ones updated by this PR and the behaviour would be inconsistent if we update some but not others. Let me know if you want this PR sooner to fix a specific bug, but otherwise I suggest also updating all symbols inside symbols/datatypes.
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.
Fantastic work @arporter. There is final documentation suggestions but then this can go.
src/psyclone/tests/psyir/frontend/fparser2_subroutine_handler_test.py
Outdated
Show resolved
Hide resolved
Also check if the sphinx-link-check issue is relevant and launch an integration test since I see none of the last ones were complete. |
Link check failure seems to have sorted itself out. I've triggered the integration tests again. Ready for another look, CI-permitting. |
LFRic 'everything-everywhere-all-at-once' integration test failed due to hitting max. recursion depth. |
The problem was that in |
Integration tests were green and coverage is all OK. Really ready for review... |
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.
@arporter All comments have been addressed, the documentation has been updated explaining the new internal details and it builds correctly. The integration passes (and the system was noisy but looking at the best bars on multiple runs it seems the performance is unaffected). This is ready to merge.
Fixes bugs in SymbolTable deep copy for for symbol initial values and shape specification. Also fixes a bug in InlineTrans when the lower bound of an array formal argument is specified by a reference to another formal argument.
Finally, adds the
force
option to InlineTrans to permit it to ignore CodeBlocks within the target routine.