-
Notifications
You must be signed in to change notification settings - Fork 72
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
api: add replace_by_if helper to control ssa value replacement #3196
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3196 +/- ##
==========================================
- Coverage 90.01% 90.01% -0.01%
==========================================
Files 431 431
Lines 54282 54294 +12
Branches 8410 8419 +9
==========================================
+ Hits 48863 48873 +10
Misses 4061 4061
- Partials 1358 1360 +2 ☔ View full report in Codecov by Sentry. |
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.
Nice! I just had a comment with the name hint change, but otherwise that's perfect!
if test(use): | ||
use.operation.operands[use.index] = value | ||
# carry over name if possible | ||
if value.name_hint is None: |
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.
Can we have an option for changing the name?
Compared to replace_by
, we don't change all uses, so we might run this function multiple times per value, making this name change a bit weird?
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.
I doubt we'll ever hit the case you describe, it seems to be primarily useful for "replace everything except" as opposed to some targeted replacing. I'll merge this PR, and happily add an option if you are sure that it would be a good idea
Resolves #3190