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

Improve hq.rs #211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve hq.rs #211

wants to merge 1 commit into from

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented May 13, 2024

Reworked hq.rs to be simpler.

I found what seems like a bug in the code when it uses v.wrapping_add(1) > 0 when v is a usize-- because the only way for this condition to be false is for v to be usize::MAX -- and this will not be consistent between x32 and x64 versions. Moreover, there is even a case of wrapping_add(2) -- which would only pass if the original is usize::MAX-1.

Given all this, this expression does not make any sense, and should be replaced with m.alloc_cell(num_bytes + 2):

if num_bytes + 2 > 0 {
    m.alloc_cell(num_bytes + 2)
} else {
    AllocF::AllocatedMemory::default()
}

@nyurik nyurik force-pushed the rework-backrefs branch 2 times, most recently from 499dc2a to 0ec80e0 Compare May 21, 2024 13:17
@nyurik nyurik force-pushed the rework-backrefs branch from 0ec80e0 to b7d66fc Compare May 27, 2024 11:29
Attention: there is clearly a bug in the code when it uses `v.wrapping_add(1) > 0` when `v` is a `usize`-- because the only way for this condition to be false is for `v` to be `usize::MAX` -- and this will not be consistent between x32 and x64 versions. Moreover, there is even a case of `wrapping_add(2)` -- which would only pass if the original is `usize::MAX-1`.

Given all this, this expression does not make any sense, and should be replaced with `m.alloc_cell(num_bytes + 2)`:

```rust
if num_bytes + 2 > 0 {
    m.alloc_cell(num_bytes + 2)
} else {
    AllocF::AllocatedMemory::default()
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant