refactor(queue): back Queue by Deque instead of a linked list - #3918
Merged
Conversation
Collaborator
Coverage Report for CI Build 5738Coverage decreased (-0.02%) to 90.427%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Replace the cons-cell linked list with a #valtype wrapper around @deque.Deque, so elements live in one contiguous circular buffer instead of a heap cell per element, and the wrapper itself is unboxed. - Most Queue methods delegate directly to the deque (push_back, pop_front/front, clear, copy, iter, from_iter, append for transfer). - each/eachi/fold keep their #locals(f) guarantee via for-in loops, since locality does not propagate across the package boundary. - Deque becomes a pub struct with priv fields: #valtype fields cannot be abstract types, and this exposes only "Deque is a concrete boxed struct" while keeping buf/len/head inaccessible. - transfer's self-transfer guard compares the inner deques physically, as wrapper identity is gone once the struct is unboxed. Queue's generated interface (pkg.generated.mbti) is unchanged.
Unboxing the Queue wrapper requires Deque to be a non-abstract pub struct, since #valtype fields cannot be abstract types. Revert that API commitment for now: Queue stays a plain boxed single-field wrapper, and the deque package is untouched by this branch again. The #valtype optimization can be revisited later.
Contributor
Review sign-off (Claude)Reviewed the full diff, rebased onto latest Semantics checked against
|
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors @queue.Queue to be implemented as a thin wrapper over @deque.Deque, replacing the previous cons-cell singly-linked-list representation while keeping the public API unchanged (as indicated by the zero-diff generated .mbti).
Changes:
- Replaced internal queue storage from
Conscells to aninner : @deque.Deque[A]buffer. - Updated core queue operations (
push,pop,peek,copy,iter,from_iter,transfer) to delegate toDequeequivalents, witheach/eachi/foldusingfor-in loops to preserve#locals(f)behavior. - Updated tests to remove
Cons-internal assertions and keep intent via higher-level equality checks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
queue/types.mbt |
Replaces the linked-list cell type with a Deque-backed Queue wrapper. |
queue/queue.mbt |
Reimplements queue operations as Deque delegations and adjusts iteration/fold and tests accordingly. |
queue/moon.pkg |
Adds moonbitlang/core/deque dependency for the new backing implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
64
to
66
| pub fn[A] Queue::clear(self : Queue[A]) -> Unit { | ||
| self.length = 0 | ||
| self.first = None | ||
| self.last = None | ||
| self.inner.clear() | ||
| } |
Comment on lines
266
to
+271
| pub fn[A] Queue::transfer(self : Queue[A], dst : Queue[A]) -> Unit { | ||
| if physical_equal(self, dst) { | ||
| if physical_equal(self.inner, dst.inner) { | ||
| return // no-op for self-transfer | ||
| } | ||
| if self.length > 0 { | ||
| match dst.last { | ||
| None => { | ||
| dst.length = self.length | ||
| dst.first = self.first | ||
| dst.last = self.last | ||
| self.clear() | ||
| } | ||
| Some(last) => { | ||
| last.next = self.first | ||
| dst.length += self.length | ||
| dst.last = self.last | ||
| self.clear() | ||
| } | ||
| } | ||
| } | ||
| dst.inner.append(self.inner) | ||
| self.inner.clear() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reimplements
@queue.Queueas a thin wrapper around@deque.Deque, replacing the cons-cell linked list. Elements now live in one contiguous circular buffer instead of a heap cell per element.How
Queuemethods are one-line delegations:push→push_back,pop/peek→pop_front/front, plusclear,length,is_empty,copy,iter,from_iter, and theQueue(arr)constructor.transferbecomesDeque::append+clear, keeping the self-transfer no-op (guarded by physical equality of the inner deques).each/eachi/foldkeep their#locals(f)guarantee by iterating withfor-in loops instead of passingfto deque's methods — locality does not propagate across the package boundary.cell_equaltest poked at the removedConsinternals; its intent (empty ≠ non-empty) is folded into theequaltest.API impact
queue/pkg.generated.mbtihas a zero diff — the public queue API is unchanged, including theabort("Queue is empty")messages ofunsafe_peek/unsafe_pop. The deque package is untouched.Deferred:
#valtypeunboxingMarking the wrapper
#valtype(soQueue[A]is represented as the bare deque reference, no wrapper allocation) was tried and works, but requiresDequeto become a non-abstractpub struct { // private fields }, because#valtypefields cannot be abstract types. That's a public representation commitment on the deque package, so it's left for a follow-up discussion. The extra cost of the boxed wrapper is one small heap object per queue instance, not per element.Verification
moon check: clean, whole repomoon test: 6775/6775 pass (wasm-gc)moon test --target native -p queue -p deque -p debug: 434/434 passmoon test --target js -p queue -p deque -p debug: 448/448 pass