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

Zig arrays to be stack-allocated #354

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

PDeveloper
Copy link

I have:

Description of changes

To be closer to the C or Fortran versions, use stack-allocated arrays in the Zig version instead of dynamically heap-allocated arrays, providing more accurate results.

@gwenzek
Copy link

gwenzek commented Jan 13, 2025

you should also remove the std.mem.eql at the top of levehenstein.
C version isn't doing that.
also I don't believe the benchmark is comparing eql strings anyway, so you're just doing more work without payout.

@@ -16,6 +15,10 @@ fn levenshteinDistance(s1: []const u8, s2: []const u8, prev_row: []usize, curr_r
const m = str1.len;
const n = str2.len;

// Initialize arrays
var prev_row: [1024]usize = undefined;
var curr_row: [1024]usize = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is broken code and will panic with some inputs. C is doing VLAs which is also broken (e.g,. very large strings that overflow the stack).

@D-Berg
Copy link
Contributor

D-Berg commented Jan 16, 2025

See this comment on why the arrays are heap allocated.

@zierf
Copy link
Contributor

zierf commented Jan 25, 2025

In order to be able to use arbitrary input sizes, stack-allocated arrays at comptime with fixed sizes are no longer an option.

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.

5 participants