Fix: replace_arith_modules ignores -slack_threshold due to key mismatch#9580
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where the -slack_threshold option in replace_arith_modules was ignored because the code was still checking for the old -slack_margin key. The change correctly updates the key name. I've added one suggestion to improve consistency by renaming the local variable from slack_margin to slack_threshold and also pointed out a minor typo and inconsistency in a related log message.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hi @maliberty, |
suhr25
force-pushed
the
fix/rsz-replace-arith-modules-slack-threshold
branch
from
de9c8cb to
cde382d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
maliberty
left a comment
maliberty
left a comment
There was a problem hiding this comment.
Thanks. Ideally we would have a test for this option.
|
Apparently these need updating |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
suhr25
force-pushed
the
fix/rsz-replace-arith-modules-slack-threshold
branch
from
4a7091b to
1e39beb
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
keys(-slack_margin) was checked but only keys(-slack_threshold) is registered by parse_key_args, so the option was always silently ignored. Signed-off-by: Suhrid Marwah <suhridmarwah07@gmail.com>
Signed-off-by: Suhrid Marwah <suhridmarwah07@gmail.com>
Signed-off-by: Suhrid Marwah <suhridmarwah07@gmail.com>
Golden files reflected the old log line that used 'replace_arith_module' (missing 's') and '-slack_margin'. Update to match the corrected log output: 'replace_arith_modules ... -slack_threshold 0.0'. Signed-off-by: Suhrid Marwah <suhridmarwah07@gmail.com>
Signed-off-by: Suhrid Marwah <suhridmarwah07@gmail.com>
Signed-off-by: Suhrid Marwah <suhridmarwah07@gmail.com>
suhr25
force-pushed
the
fix/rsz-replace-arith-modules-slack-threshold
branch
from
1e39beb to
62fd8dd
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hi @maliberty, |
2 participants
SUMMARY
This PR fixes a bug in
replace_arith_moduleswhere the-slack_thresholdoption was being ignored due to a mismatched key name. The issue is limited tosrc/rsz/src/Resizer.tcland only affects Tcl argument parsing, with no impact on the C++ layer.FIX / CHANGES
Root cause:
The option was renamed from
-slack_marginto-slack_threshold, but the later checks still used the old name. As a result, the user-provided value was never picked up and always defaulted to0.0.Approach:
Updated the key name in both the guard and parsing call so it matches what
parse_key_argsproduces.Fix
- if { [info exists keys(-slack_margin)] } { - set slack_margin [rsz::parse_time_margin_arg "-slack_margin" keys] + if { [info exists keys(-slack_threshold)] } { + set slack_margin [rsz::parse_time_margin_arg "-slack_threshold" keys]VERIFICATION
-slack_margin 0.5instead of0.0