refactor: deslopify the crate inside and out - #345
Merged
Merged
Conversation
No caller exists in any feature combination: the method was added for a broadcast consumer that has since been removed, and the module-level #[allow(dead_code)] on the arena module hid that. Iteration over occupied values is covered by take_all and into_iter.
Nothing constructs an Arena through Default: every mem::take site in the crate takes a WakerSet, which has its own Default impl built on WakerSet::new. WaitList already sets the precedent of a const fn new without a Default impl, so new_without_default stays quiet.
Its only callers are WaitList::occupied_len, which is itself #[cfg(test)], and the arena unit tests. Production code observes occupancy through is_empty, which stays available in all builds.
The module itself is a private mod declaration, so crate visibility is already restricted at the module boundary. Repository style keeps restricted visibility on the boundary and plain pub on the module's items, the same pattern the internal module follows.
These comments narrate the very line they precede instead of explaining
a rationale: field docs that paraphrase the field's type ("Container
storing the protected data" on UnsafeCell<T>, "Non-null pointer to the
mapped data" on NonNull<T>), the "ownership certificate" voice-over on
Arc lock fields, "Release the lock by calling release on the semaphore"
above s.release(1), and the "Use DerefMut ..." meta commentary. The
broadcast Shared::inner field doc additionally duplicated the Inner
type's own documentation. Comments carrying real invariants (variance,
non-zero max_readers) are kept.
"We safely extract the Arc from the ManuallyDrop guard" just asserts safety in the imperative mood without giving a reason, and OwnedMutexGuard::map had no comment at all while its siblings did. Every extraction site now states the actual invariant, matching the wording the rwlock write guards already used: the source guard is ManuallyDrop and will not be dropped, so moving the Arc out transfers lock ownership instead of releasing it.
Every access to RwLockWriteGuard and OwnedRwLockWriteGuard fields happens inside the file that defines the guard, so the pub(super) widens visibility without a consumer. The read guard fields keep theirs: downgrade constructs read guards from the write guard modules.
"large enough while not touch the edge" was ungrammatical and named no constraint: the cap exists to keep semaphore permit arithmetic away from usize::MAX while remaining unreachable in practice.
…itional The Drop impls matched on the fetch_sub result with a no-op fallback arm whose only content was a comment saying there is nothing to do. An equality check expresses the last-sender test directly, the same shape the mpsc sender and waitgroup disconnect paths already use. The bounded sender keeps its comment explaining why only receivers need waking.
The reclaim_vacated comment claimed the buffer grows only in Backlog::publish, but the growth is the push_back in publish_retained, which the bounded channel calls directly, bypassing publish entirely. The accounting argument only holds if the reference names the actual growth point.
The queue is single-producer: send borrows the sender exclusively, so at most one Send future exists and send_waiters can hold only its own node. Once Drop removes that node, notify_one always finds an empty list and the "another blocked sender" scenario the field comment describes cannot occur. The mpmc queue keeps its handoff because multiple producers make it live; the receiver-side handoff here stays for the same reason. The removed branch could only fire for a mem::forget-leaked send future, where waking the stale waker is a pointless poll.
Both constructors claimed operations acquire "internal mutexes", but the queue state lives behind one Mutex, and the unbounded paragraph even contradicted itself two sentences later. The mpmc and mpsc modules already say "an internal mutex".
The struct summary promised std-style "initialized at most once" semantics, but take() deliberately moves the cell back to an uninitialized state so it can be initialized again; the module-level docs already describe the actual contract. The set() docs also said "thread" where every sibling method says "task".
let _permit = permit extended nothing: a by-value parameter already lives until the end of the function. It is scaffolding from the pre-ValueCell implementation, when the body held the permit across several statements; naming the parameter _permit expresses the held-but-unused intent directly and the SAFETY comment already documents the permit invariant.
OwnedLatchWait duplicated LatchWait field-for-field and poll-for-poll, differing only in holding Arc<Latch> instead of &Latch, and the intern_poll helper existed only to be shared by the two. Both event primitives already implement the identical method as self.wait().await: the Arc lives inside the async block while wait() borrows from it, and cancel-safety is unchanged because the same LatchWait future drives the wait. With a single caller left, intern_poll is inlined back into LatchWait::poll.
Every other future type in the crate uses the message with `.await` in backticks; this one diverged by the missing markup alone.
…ject The struct is private and constructed exactly once, inside Pool<T, M>::get where M is inferred from the pool; neither impl block relies on the default. It was copied from the public Object<T, M = NeverManageObject<T>> signature, where the default backs the Pool<T> convenience API, and the bounded pool's UnreadyObject never had one.
Both call sites live in the pool subtree (bounded.rs, unbounded.rs), so crate-wide visibility overshoots the consumers. pub would leak the methods into the public API because ObjectStatus is re-exported, and pub(super) is the idiom sibling modules already use for exactly this case, e.g. watch's SendError::new.
The subcommand invokes cargo test --workspace, which runs unit tests, the tests-integration suites, and doc tests; "Run unit tests" under-reports the workflow that AGENTS.md designates as the source of truth. The sibling entries already say "workspace" explicitly.
The table is read only by cargo-release, which this repository does not use: the release skill publishes with cargo publish through ATR and Trusted Publishing, and nothing in the repo consumes the key. The package is already unpublished via publish = false, which is what the other private workspace members rely on.
The asyncband/ copy of MIGRATE.md ships inside the published crate and is also what the GitHub directory view renders, but its relative CHANGELOG.md and CHANGELOG-OLD.md links resolve against asyncband/, where no changelogs exist, and 404 there as well as in the packaged crate. Absolute links work in every context; the two copies stay byte-identical.
The workspace declarations for hashbrown and ureq already set default-features = false, so repeating it in the member manifests is a no-op; cargo tree confirms the resolved feature sets are unchanged.
This reverts commit c2618fa.
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
1 participant
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.
Summary
A deslopify sweep of the crate: one commit per finding, each commit message explains why the removed or rewritten piece was slop. Net -84 lines, no behavior change.
Arena::valuesand the unusedDefault for Arena, gateArena::lento tests, and aligntest_supportvisibility with the module-boundary convention.Arcextraction site, make the write-guard fields private, and explain the default reader cap accurately.Dropimpls into a conditional, fix thereclaim_vacatedgrowth-point reference, and drop field docs duplicatingInner's own doc.Send::dropwaiter handoff that is unreachable in a single-producer queue, and say "an internal mutex" (there is exactly one).take()re-initializes), say "task" not "thread", and remove the vestigiallet _permit = permit;rebind.wait_ownedby awaitingwait(), deleting the field-for-field duplicatedOwnedLatchWait(the pattern both event primitives already use).UnreadyObjectand narrowObjectStatusmutation methods topub(super).cargo x testas running workspace tests, remove the unused cargo-release metadata, make the packaged MIGRATE.md changelog links absolute, and drop redundantdefault-features = falseon workspace-inherited deps.Considered and left alone:
AutoResetEvent::poll_waithand-rollsregister_waker: adopting the shared helper requires reshaping theWaiterenum (the helper wants anOption<Waker>slot); state-machine churn for three lines, fine as a follow-up if wanted.oneshot::Receiver's explicitimpl Unpin: redundant with the auto impl today, but it matches thempsc::UnboundedReceiverconvention guarding the auto-trait contract.TaskBatchduplication between mpmc and spmc: deduplicating means restructuring the fixture generics (theClonesender bound), a design change rather than slop removal.Validation:
cargo x lint,cargo x test,cargo x check(feature matrix), andcargo x miriall pass.