-
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
Debugging linter rules #116
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/116/index.html |
33432bc
to
a0b8e0b
Compare
Codecov Report
@@ Coverage Diff @@
## main #116 +/- ##
==========================================
+ Coverage 92.12% 92.15% +0.03%
==========================================
Files 89 90 +1
Lines 16494 16650 +156
==========================================
+ Hits 15195 15344 +149
- Misses 1299 1306 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
a0b8e0b
to
16f022a
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.
This is fantastic! Thanks so much and a perfect start to what I imagine could grow into a quite handy collection of similar utilities.
My main request would be to document this in the loki-lint documentation: https://github.com/ecmwf-ifs/loki/blob/main/docs/source/loki_lint.rst
Otherwise a few comments and questions but nothing that would stop this from going in.
""" | ||
|
||
# first resolve associates | ||
resolve_associates(subroutine) |
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.
[no action required] Just a thought on design: We should probably expose somehow whether a linter rule is changing the IR (and, optionally, call these rules with a cloned IR to ensure rules applied subsequently in the same linter pass get to see the original IR.
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.
Agreed, great idea!
fe8f704
to
366889d
Compare
366889d
to
cb73473
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.
Thanks, very happy with this now.
continue | ||
|
||
arg_map = {carg: rarg for rarg, carg in call.arg_iter()} | ||
for arg in arg_map: |
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.
You could even go further and use for carg, rarg in arg_map.items()
to save the arg_map[arg]
lookups in the loop body. But mentioning this only for educational purposes.
This PR adds the following two linter rules:
ArgSizeMismatchRule
: check for size discrepancies between arguments and dummy argumentsDynamicUboundCheckRule
: check for dynamicUBOUND
checks for assumed-shape dummy argumentsOther changes include:
GenericRule.check
andGenericRule.check_subroutine
API to allow optional passing ofscheduler
targetslint_rules/tests
toconftest.py