0xMiden / 0xMiden/miden-vm

word::gt / word::lt: internal accumulator variable is mislabeled in comments (swapped between the two procedures)

未关闭
#3,676 5 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
documentation good first issue
主要语言
Rust
星标
772
派生
352
平均合并
1 天 12 小时
30 天内合并 PR
93

描述

## Summary

`word::gt` and `word::lt` in `crates/lib/core/asm/word.masm` are near-identical
procedures (the doc comment on `lt` even says *"this is exactly the same as
`word::gt` except it uses `lt` rather than `gt`"*). The underlying logic in
both is correct, but the internal accumulator variable is mislabeled in the
inline comments, in a way that's swapped between the two procedures — almost
certainly a copy-paste artifact from when one was derived from the other.

- Inside `gt` (which computes `is_lhs_greater`), the accumulator is commented
as `is_lhs_less`.
- Inside `lt` (which computes `is_lhs_less`), the accumulator is commented as
`is_lhs_greater`.

## Why I'm flagging this despite the logic being correct

I don't want to waste anyone's time with a false bug report, so before
opening this I verified the actual logic is correct: I wrote a small
Miden-stack simulator (`movup`/`movdn`/`dup`/`swap`/`eq`/`lt`/`gt`/`and`/`or`)
and ran both procedures' exact instruction sequences against the reference
`is_word_greater` algorithm from the doc comment, across 10 test vectors
(equal words, differing at each of the 4 limb positions, all-zero, etc.) —
all 20 checks (`gt` + `lt`) matched. So this is purely a labeling issue, not
a functional bug.

Given that, I still think it's worth fixing: mislabeled variable names in
comparison-operator internals are exactly the kind of thing that could
mislead someone modifying or auditing this code later (e.g. "wait, why does
`gt` track something called `is_lhs_less`?").

## Where

`crates/lib/core/asm/word.masm`, in the `push.1.0` initialization comment and
the accumulator-tracking comments throughout the loop body of:
- `gt` (~line 94 onward) — comments say `is_lhs_less`, should say
`is_lhs_greater`
- `lt` (~line 178 onward) — comments say `is_lhs_greater`, should say
`is_lhs_less`

## Suggested fix

Swap the two mislabeled names back to match what each procedure actually
computes. Happy to open a PR for this if assigned — didn't want to submit
one without an issue/assignment per the contributing guidelines.

贡献指南

打开贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。