-
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 #313) permit intrinsic shadowing #418
Conversation
Looking at the test failures and (lack of) code changes, it's clear that this branch needs refining so that it only permits a mismatch in arguments if an intrinsic has been (or could be?) overridden. |
The trouble is that (in NEMOVAR), there isn't always an explicit import of the overridden intrinsic. Obviously, if we find a wildcard import and the tentative intrinsic match has an invalid number of arguments then we know it can't be an intrinsic and must be from elsewhere. However, if it happens to have the same number of arguments as the intrinsic then we will always match an Intrinsic, even if those arguments are of the wrong type. (In future we could check the type of the arguments.) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #418 +/- ##
==========================================
+ Coverage 91.96% 91.99% +0.02%
==========================================
Files 85 85
Lines 13644 13681 +37
==========================================
+ Hits 12548 12586 +38
+ Misses 1096 1095 -1 ☔ View full report in Codecov by Sentry. |
This has been needed for NEMOVAR for a while: currently we are over zealous in checking the number of arguments to intrinsics and fell foul of the fact that NEMOVAR overrides DOT_PRODUCT with something that can accept more args than the Fortran intrinsic can. This PR alters the parser's behaviour so that if the number of arguments don't match and there's somewhere that a symbol could be coming from then we don't match an intrinsic. I had to extend SymbolTable with a new |
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.
Good job. I have mostly minor comments but you will see that I am a little concerned as to whether this solution works in situations other than module/subroutine.
I hadn't considered submodules so thanks for spotting that. Having done some reading I see that (as you'd expect) they have access to all the symbols made available by the 'parent' module. I'll have to allow for that. |
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.
Looks good. Just some minor requests.
Ready for another look now... |
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.
Changes look good. However, there appears to be a line that is not covered by the tests. Could you please check and fix if required.
Thanks for spotting that missed line - I'd been fooled by Codecov reporting success. Happily that line was associated with #170 (which is now fixed) and so I've been able to simplify it. |
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.
As discussed with @arporter I took over this review in order to add this functionality to the next release. I see that @rupertford was already happy with how his previous comments were addressed and just one line on the codecov was missing. So I just gave it a quick read and checked that the codecov is now complete. This PR is ready to merge.
No description provided.