Skip to content
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

InlineMember: rename duplicate locals in the body #172

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

rolfhm
Copy link
Contributor

@rolfhm rolfhm commented Oct 13, 2023

The shadow mapper will only find variables in the declaration and leaves the body unchanged.

I do not understand the need for the [dl.name for dl in duplicate_locals] construct though. I would have thought that a v in duplicate_locals or v.name in duplicate_locals would be sufficient.

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #172 (6b3bcb3) into main (af8ec9f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #172   +/-   ##
=======================================
  Coverage   92.14%   92.14%           
=======================================
  Files          90       90           
  Lines       16690    16695    +5     
=======================================
+ Hits        15379    15384    +5     
  Misses       1311     1311           
Flag Coverage Δ
lint_rules 96.22% <ø> (ø)
loki 92.11% <100.00%> (+<0.01%) ⬆️
transformations 91.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
loki/transform/transform_inline.py 87.68% <100.00%> (+0.46%) ⬆️

@reuterbal reuterbal changed the title transform body fix InlineMember: rename duplicate locals in the body Oct 17, 2023
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding and fixing this. I have a small remark regarding the implementation, otherwise fine from my side.

loki/transform/transform_inline.py Outdated Show resolved Hide resolved
@rolfhm rolfhm requested a review from reuterbal October 27, 2023 07:09
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this. Looks good to go!

@reuterbal reuterbal added the ready to merge This PR has been approved and is ready to be merged label Nov 9, 2023
@reuterbal reuterbal merged commit 874ac83 into ecmwf-ifs:main Nov 9, 2023
7 of 11 checks passed
@rolfhm rolfhm deleted the sbrm-inline-transform-fix branch November 24, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants