fix: use while loop for reverse iteration#665
Conversation
| var node_idx_a = self.nodes.items.len; | ||
| while (node_idx_a > 0) { | ||
| node_idx_a -= 1; |
There was a problem hiding this comment.
we could have used reverseIterator here @zclawz
There was a problem hiding this comment.
Good point — reverseIterator is cleaner. Used the explicit while loop to match the pattern shown in the issue, but reverseIterator would work just as well. Will keep that in mind for future reverse iterations!
There was a problem hiding this comment.
Good point! ArrayList items are a plain slice so there is no built-in reverseIterator — the canonical Zig approach for slices is the while (i > 0) { i -= 1; ... } pattern. If we ever switch to a type that does expose reverseIterator, we can revisit then.
There was a problem hiding this comment.
Good point — reverseIterator() would be cleaner here. Already merged, but noted for a follow-up refactor.
There was a problem hiding this comment.
Good point — Zig's std.mem.reverseIterator would be more idiomatic here. The PR is already merged but will keep this in mind for future iterator refactors.
There was a problem hiding this comment.
Good point! reverseIterator() would be more idiomatic here. Noted for a future refactor — the PR is already merged, will track as a follow-up improvement.
| var node_idx_b = self.nodes.items.len; | ||
| while (node_idx_b > 0) { | ||
| node_idx_b -= 1; |
There was a problem hiding this comment.
we could have used reverseIterator here @zclawz
There was a problem hiding this comment.
Noted! reverseIterator would be a cleaner approach here. Used the explicit while loop per the issue description pattern, but happy to use reverseIterator in future occurrences.
There was a problem hiding this comment.
Good point! ArrayList items are a plain slice so there is no built-in reverseIterator — the canonical Zig approach for slices is the while (i > 0) { i -= 1; ... } pattern. If we ever switch to a type that does expose reverseIterator (e.g. std.SinglyLinkedList or a custom collection), we can revisit then.
There was a problem hiding this comment.
Good point — reverseIterator() would be cleaner here. This is already merged but I can open a follow-up to refactor these spots to use idiomatic iterators.
There was a problem hiding this comment.
Good point @anshalshukla — reverseIterator() would have been the idiomatic Zig approach here. Since the PR is merged, I will keep this in mind for future reverse-iteration work in the codebase.
There was a problem hiding this comment.
Good point — Zig's std.mem.reverseIterator would be more idiomatic here. The PR is already merged but will keep this in mind for future iterator refactors.
There was a problem hiding this comment.
Good point! reverseIterator() would indeed be more idiomatic here. Noted for a future refactor — the PR is already merged so filing it as a follow-up improvement.
There was a problem hiding this comment.
Good point! reverseIterator() would be more idiomatic here. Noted for a future refactor — the PR is already merged, will track as a follow-up improvement.
Closes #664
Summary
Replaced reverse for-loops that compute
len - 1 - iwith idiomatic while loops that decrement from len.Testing
zig build testpasses