Use this checklist to guide a review, not to reject code mechanically. Start with what the code must do: its inputs, outputs, failure behavior and invariants. Then inspect the API and implementation. A clear loop that solves the wrong problem still needs a correctness fix.
When leaving feedback, identify the reason: a bug, an API constraint, a measured cost, an untested performance hypothesis or a style preference.
1. Does ownership match the API's purpose?
Prefer &str when a function needs string contents, and &[T] when it needs
a sequence. They accept borrowed views without requiring a particular
owning container. A &String or &Vec<T> can be appropriate when the
function specifically needs that container's properties, such as capacity.
Ask what each parameter needs to permit:
- A value parameter can transfer ownership. It does not necessarily allocate,
and
Copyvalues can be copied rather than moved. - A shared reference provides shared access. Interior-mutable types can still change through it.
- A mutable reference provides exclusive access. It can support mutation, but does not promise that the function will mutate the value.
Taking a String by value does not force a fresh allocation if the caller
already owns one. Conversely, converting &str to String can be justified
when the callee must retain independent data. Review the intended lifetime
and reuse of that data before recommending a signature change.
For generic APIs, consider whether callers need named type parameters or
relationships between parameter types. Argument-position impl Trait is
convenient, but that anonymous parameter cannot itself be explicitly
supplied with turbofish syntax. A named generic parameter allows that.
Return-position impl Trait hides the concrete type's name, but auto traits
such as Send can still affect callers. Changing the hidden type may change
whether the result can cross a thread boundary.
2. Are allocations and copies serving a purpose?
Look at clone, to_owned, formatting and intermediate collections. Ask
what each operation preserves or makes possible before removing it.
- Compare borrowed strings directly when independent owned copies are not needed for anything else.
- Reuse buffers when repeated allocation matters, while considering retained capacity after a large input.
- Reserve capacity when a useful size estimate is available. Iterator
size_hintmay already help collection; filtering can weaken the lower bound without making every size hint useless. - Inspect a collection that is immediately consumed again. It may be avoidable, or it may intentionally separate borrows, preserve a snapshot, sort data or change when errors and side effects happen.
- Consider
Cow<str>when a result sometimes borrows and sometimes owns. It adds an ownership choice to the API, so use it when callers benefit.
Cloning inside a loop is not automatically a mistake. Cloning an Rc,
duplicating a small value and copying a large buffer have different costs
and semantics. Measure the operation that matters to the application.
3. Is control flow easy to follow?
Use iteration over elements when indices add no meaning. Keep an index loop when position, neighboring elements or coordinated access is central to the algorithm. Readability depends on the complete operation, not chain length.
if let suits one branch of interest. let ... else suits extracting a
value when the alternative exits the current path. Use match when several
branches deserve explicit treatment, and matches! for a pattern test.
Use ? when returning early with an error matches the API. A match may
instead need to recover, add context, retry or continue processing other
inputs. Early returns often reduce nesting, but verify what cleanup and
partial state remain on each exit.
4. Do names and types communicate the contract?
Conversion names follow conventions, not allocator guarantees:
| Form | Typical meaning | Review question |
|---|---|---|
as_ |
Cheap view or conversion | Is the cost consistent with a cheap operation? |
to_ |
Conversion that retains the source | Does the caller need the new representation? |
into_ |
Conversion that consumes the receiver | Is ownership transfer useful here? |
from |
Conversion or construction from an input | Is this infallible, or should it be TryFrom? |
new / with_* |
Construction, sometimes with a named configuration | Are arguments and defaults clear? |
For example, integer to_le_bytes() returns an array without a heap
allocation. String::into_bytes() reuses the string's storage, while other
consuming conversions may allocate. new can take arguments. Document a
meaningful cost rather than relying on a prefix to promise one.
Use familiar names such as len and is_empty for collection-like APIs.
Getters commonly use the field name; _mut commonly identifies mutable
access. Check consistency with the surrounding API instead of renaming
methods solely to satisfy a slogan.
5. What do the lints establish?
Run the project's configured Clippy checks and inspect each diagnostic. Clippy groups include correctness, suspicious, style, complexity, performance, pedantic, restriction and nursery checks. Defaults and group membership vary by lint and toolchain, so consult the relevant lint's documentation rather than memorizing a fixed list.
-D warnings promotes enabled warnings to errors; it does not enable every
allow-by-default lint. Select restriction lints individually according to
project policy. Enabling the entire restriction group can introduce
conflicting requirements.
A lint can reveal a bug or suggest a clearer expression. Its absence does not establish correctness, and a suppression needs a reason that applies to the specific code. Tests, measurements and invariant checks still matter.
6. Which design choices need evidence?
Sorting: sort_by_cached_key computes a key at most once per element
and stores keys temporarily. It may help when key extraction is expensive;
caching also has costs. sort_by_key can compute a key repeatedly.
sort_unstable avoids auxiliary heap allocation but does not preserve
equal elements' relative order. Check whether tie order is observable.
Containers: compare lookup patterns, mutation frequency, iteration order, key costs and input size. A small vector may outperform a hash map, but no fixed element count guarantees that result on every workload.
Enums and traits: an enum can represent a closed set of alternatives
with exhaustive matching. Trait-based APIs support independently defined
implementations; they can use generics, borrowed trait objects or boxed
trait objects. Extensibility does not by itself require Box<dyn Trait>.
An enum's fields can allocate too.
Shared mutation: Rc<RefCell<T>> may suit a single-threaded graph or
event-driven model. Check ownership cycles, borrow duration and reentrant
callbacks. borrow and borrow_mut can panic on conflicts; try_borrow
and try_borrow_mut let the caller handle failure. Alternatives include
ownership restructuring and index-based storage, without necessarily
introducing unsafe code.
Layout: changing field types or storage structure can reduce memory use, but can also change ownership, locality, mutation costs and API behavior. Measure representative data rather than treating a smaller handle as the whole memory budget.
Errors: choose an enum when callers need to distinguish defined cases. An erased error type may suit an API that forwards varied failures. Consider recovery, context, thread-safety bounds and compatibility rather than deciding solely from the label "application" or "library".
Make feedback actionable
Name the concrete effect and suggest a way to verify it. For example: "This cached string is cloned on every lookup. Can we return a borrow whose lifetime fits the caller, and measure the change on the lookup benchmark?"
Keep preferences separate from required fixes. A useful review helps the author understand the behavior and trade-off behind each proposed change.