Linter debug rule fixes #126
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/126/index.html |
Codecov Report
@@ Coverage Diff @@
## main #126 +/- ##
==========================================
- Coverage 92.09% 92.07% -0.03%
==========================================
Files 89 89
Lines 16540 16537 -3
==========================================
- Hits 15233 15226 -7
- Misses 1307 1311 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
lint_rules/lint_rules/debug_rules.py
Outdated
| if not stat: | ||
| dummy_size = Product(SubstituteExpressions(assign_map).visit(dummy_arg_size)) | ||
| # we check for a maximum of two indirections for the dummy and arg dimension names | ||
| for _ in range(2): |
There was a problem hiding this comment.
Can we make this indirection depth a config value? That would allow overwriting the value from outside if necessary. See for example
loki/lint_rules/lint_rules/ifs_coding_standards_2011.py
Lines 36 to 38 in 5ed914c
There was a problem hiding this comment.
Aah that is a great idea! Please wait to merge until I've added this.
| op_str = op if op != '!=' else '/=' | ||
| line = [line for line in lines if op_str in strip_inline_comments(line.string)] | ||
| if not line: | ||
| _op_map = { |
There was a problem hiding this comment.
Just curious: any particular reason for moving the map here?
There was a problem hiding this comment.
As explained in #118, this ended up here because I misunderstood one of your earlier comments 😅
awnawab
force-pushed
the
naan-debugrules-fix
branch
from
5ed914c
to
f6a3ba7
Compare
lint_rules/lint_rules/debug_rules.py
Outdated
| @@ -284,13 +303,24 @@ def fix_subroutine(cls, subroutine, rule_report, config): | |||
| vtype = arg.type.clone(shape=new_shape, scope=subroutine) | |||
| new_vars += as_tuple(arg.clone(type=vtype, dimensions=new_shape, scope=subroutine)) | |||
|
|
|||
| #TODO: add 'VariableDeclaration.symbols' should be of type 'Variable' rather than 'Expression' | |||
| # simplify variable declarations | |||
| single_variable_declaration(subroutine) | |||
There was a problem hiding this comment.
Ok, apologies if I'm misreading this, but from what I understand, fix_subroutine is the auto that is intended to "correct" small violations for code that gets put back into source, right? With that in mind, splitting variable declarations looks like a very invasive change for a linter! I know we like these for GPU-specific transformations, but forcing this into every file of a large code base for mere convenience seems like a pretty hard no, if we want to bring "fixed" code back into any repo. Is this really intended here, or am I misunderstanding this?
There was a problem hiding this comment.
Your understanding is correct....this was an easy yet robust way to deal with any potential corner case of array declarations. I thought it would be ok given how invasive this fixer method is already.
Thanks a lot for pointing this out, I'll stop being lazy and make the fixer method more sophisticated 😅
…ections of arg size names
…ns wherever possible
awnawab
force-pushed
the
naan-debugrules-fix
branch
from
f6a3ba7
to
e5d2c6d
Compare
A small PR to fix errors in the linter debug rules:
ArgSizeMismatchRule: Check up to a maximum of two indirections for dimension names when comparing arg/dummy-arg sizesDynamicUboundCheckRule: Fixer method returnsnode_mapto trigger source invalidationDynamicUboundCheckRule: Fixer method preserves subroutine signature when updating argument declarationsFortran90OperatorsRule: Class member was added unnecessarily during removal ofExpression.source. This is now a local object.NB: This PR is stacked on top of #118 because the
DynamicUboundCheckRulefixer method relies on the removal ofExpression.source.