C++ Code Review Checklist: 20 Checks for Ownership, const-Correctness, Exception Safety and Concurrency

A checklist is a starting point, not a substitute for judgment. Every item below maps to a real class of C++ bug that a compiler will happily let through, and the point of this post is to explain why each check exists — what specifically goes wrong when it’s skipped, and why “the diff looks fine” is a weaker signal in C++ than in almost any other mainstream language. C++ gives reviewers no safety net: no garbage collector to paper over a dangling pointer, no runtime bounds checking by default, and no language-level ownership tracking outside of what the type system happens to express. That means a code review is often the last line of defense before a bug ships, which is a heavier burden than code review carries in, say, Python or Java.

Why “it compiles and looks right” is not enough

The single biggest failure mode in C++ code review is treating a clean diff as a correct diff. A pull request can pass every static analyzer, compile without warnings, and read cleanly top to bottom, and still contain a use-after-free, a data race, or a broken exception-safety guarantee — because none of those bugs are visible as syntax. They’re visible only as violations of an invariant that exists in the reviewer’s head (or doesn’t, if the reviewer doesn’t know the invariant). This is why the checklist below groups items by the kind of invariant they protect, rather than by superficial code style, and why each item comes with the failure mode it’s guarding against instead of just “do this.”

Memory Safety (5 Items)

Memory safety issues are the category C++ is most (fairly) criticized for, and they’re also the category where “looks correct” fails hardest. A leaked allocation, a dangling pointer, or an out-of-bounds write can sit dormant for months before a specific code path triggers it in production — often under a load pattern nobody exercised in testing. The fix pattern across all five items below is the same: prefer types that own their resources and enforce invariants at construction, rather than raw operations a human has to remember to pair correctly every single time.

Check for Memory Leaks

// ❌ Leak
void bad() {
    int* ptr = new int[100];
    // No delete[]!
}

// ✅ RAII
void good() {
    vector<int> v(100);  // Automatically released
}

A leak review isn’t just “did they call delete.” It’s “does every exit path from this function — including early returns and thrown exceptions — reach the delete.” That second question is why new/delete pairs are fundamentally harder to review than they look: a function can have a correct-looking delete at the bottom and still leak on three of its five return paths. RAII sidesteps the whole class of bug by tying the deallocation to a destructor that runs unconditionally during stack unwinding, so there’s no path to audit — the compiler guarantees it runs.

Dangling Pointers

// ❌ Risky
int* bad() {
    int x = 10;
    return &x;  // Returning address of a local variable!
}

// ✅ Safe
int good() {
    int x = 10;
    return x;  // Return value
}

This particular example is easy to catch because it’s obviously wrong in five lines. The dangerous version of this bug shows up across a call boundary: a function stores a pointer or reference to a caller-owned object, the caller’s object goes out of scope (or is moved, or is reallocated as part of a vector growing), and the stored pointer silently becomes garbage. Reviewing for this means asking “whose lifetime does this pointer depend on, and is that lifetime guaranteed to outlive every use of the pointer” — a question that requires tracing ownership across the whole call graph, not just the function in front of you.

Double Delete

// ❌ Crash
int* ptr = new int(10);
delete ptr;
delete ptr;  // Double delete!

// ✅ Set to nullptr
int* ptr = new int(10);
delete ptr;
ptr = nullptr;
delete ptr;  // Safe

Double delete is what happens when ownership isn’t singular — two code paths both believe they’re responsible for freeing the same pointer. It’s rarely this obvious in a real diff; it usually happens when a pointer is stored in two places (a cache and an owning container, say) and both go through cleanup on shutdown. The nullptr pattern shown here is a band-aid, not a fix — delete nullptr is a documented no-op, so it prevents the crash, but it doesn’t answer the actual review question, which is “why did two owners think they had this pointer.” A unique_ptr makes that question unaskable because the type system enforces single ownership at compile time.

Array Out-of-Bounds Access

// ❌ Risky
int arr[10];
arr[10] = 5;  // Out-of-bounds access!

// ✅ Check
if (index < 10) {
    arr[index] = 5;
}

// ✅ Use vector
vector<int> v(10);
v.at(10) = 5;  // Throws exception

operator[] on both raw arrays and std::vector is unchecked by design — it exists specifically for the case where the caller has already proven the index is valid and doesn’t want to pay for a redundant check. That means an out-of-bounds operator[] access is undefined behavior, not a guaranteed crash: it might silently corrupt adjacent memory, it might work fine in a debug build and fail in release, and it might pass every existing test because the specific index that overflows never gets exercised. .at() trades that ambiguity for a guaranteed, catchable std::out_of_range exception at the cost of a bounds check per access — the right default unless a profiler has proven the check matters in a hot loop.

Use Smart Pointers

// ❌ Raw pointer
int* ptr = new int(10);
// ... complex logic ...
delete ptr;  // Easy to forget

// ✅ Smart pointer
auto ptr = make_unique<int>(10);
// Automatically released

This is really the general form of items 1–4: nearly every memory-safety bug in this list disappears if ownership is expressed with unique_ptr or shared_ptr instead of a raw pointer plus a mental note. But the review question that matters here is subtler than “did they use a smart pointer” — it’s “does a raw pointer parameter or return value in this diff represent a transfer of ownership, or just a non-owning observer.” C++ gives no syntactic distinction between the two; Foo* getFoo() looks identical whether the caller now owns the returned object or is just borrowing a reference to something that outlives the call. A reviewer has to ask that question explicitly for every raw pointer that crosses a function boundary, because the compiler will never ask it for you.

The ownership-transfer question every raw pointer should raise

This deserves its own section because it’s the single most common thing I’ve seen slip through review. When a function signature takes or returns a raw pointer — void registerCallback(Widget* w), Node* detach() — there are at least four possible ownership contracts hiding behind identical syntax: the callee borrows it and it must outlive the call; the callee takes ownership and will eventually delete it; the caller retains ownership and the callee must never delete it; or ownership is shared in some implicit, undocumented way that nobody has actually verified. None of these are visible in the type Widget*. They’re only visible in a comment, a naming convention, or — most often — in the reviewer’s assumption about how the codebase “usually” works, which is exactly the assumption that breaks the moment someone refactors the caller six months later without touching the callee.

The fix in new code is to make the contract explicit in the type: std::unique_ptr<Widget> for a transfer of ownership, std::shared_ptr<Widget> for shared ownership, a raw pointer or reference only for non-owning access with a lifetime the caller controls, and gsl::not_null or a plain reference where null isn’t a valid state. But most C++ codebases have decades of raw-pointer APIs that predate this convention, which means the review question “does this callee now own the pointer, and did the ownership assumption change from what it was before this diff” has to be asked by hand, every time, especially on any diff that touches a function’s parameter types or return type without touching its name.

flowchart TD
    A["Raw pointer crosses<br/>a function boundary"] --> B{Who owns it<br/>after the call?}
    B -->|"Callee borrows,<br/>caller still owns"| C["Non-owning reference<br/>or raw pointer — OK<br/>if lifetime is guaranteed"]
    B -->|"Callee takes<br/>ownership"| D["Should be<br/>unique_ptr&lt;T&gt;"]
    B -->|"Ownership<br/>is shared"| E["Should be<br/>shared_ptr&lt;T&gt;"]
    B -->|"Unclear /<br/>undocumented"| F["Review blocker:<br/>ambiguous contract"]
    F --> G["Bug risk:<br/>double-free or<br/>use-after-free"]

A first-person story: the use-after-free that passed review

I once approved a PR that, on the diff itself, was completely correct. It replaced a shared_ptr<Session> member with a raw Session* cached from a connection pool, with a comment explaining that the pool “always outlives the handler” — which was true, at the time. The diff was small, the logic was sound, and every test passed. What the diff didn’t show — because it wasn’t part of the diff — was that a separate, unrelated change landed the same week that let the connection pool evict idle sessions under memory pressure. Nothing in either PR was wrong in isolation. The bug was that the ownership assumption baked into the first PR’s comment silently stopped being true because of a change in a completely different file, and no reviewer connected the two, because reviewing a diff means reviewing what changed, not re-verifying every invariant the rest of the codebase depends on. We caught it three weeks later as an intermittent use-after-free that only reproduced under load, and the fix was exactly what item 5 above recommends: replace the raw pointer with a shared_ptr so the session’s lifetime is guaranteed by reference counting instead of by a comment. The lesson I took from it isn’t “read every comment more carefully” — it’s that any raw pointer whose safety depends on an invariant maintained elsewhere in the codebase is a standing liability that a code review, by its nature, is poorly positioned to catch, because a review’s context window is the diff, not the whole system.

Performance (5 Items)

Performance review in C++ is where I see the most miscalibrated effort — reviewers spend time on micro-optimizations that the compiler already performs (or that don’t matter outside a hot loop) while missing algorithmic issues that actually cost real time in production. The default review posture should be: flag anything that changes algorithmic complexity or introduces an obviously avoidable copy of non-trivial data, but don’t block a PR over a std::string versus std::string_view choice unless a profiler has shown it matters. Premature micro-optimization has a real cost too — it makes code harder to read for a benefit that’s often unmeasurable.

Avoid Unnecessary Copies

// ❌ Copy
void process(vector<int> data) {  // Copy!
    // ...
}

// ✅ Use reference
void process(const vector<int>& data) {
    // ...
}

The rule of thumb is: if the function only reads the argument, take it by const&; if the function needs its own copy to mutate or store, take it by value and let the caller decide whether to copy or std::move into it. The by-value-plus-move pattern is often better than const& when the function is going to store the argument anyway, because it lets a caller that already has a temporary or an rvalue avoid the copy entirely — a detail that’s easy to miss if the reviewer only checks “reference used, good” without asking what the function actually does with the parameter afterward.

Use reserve

// ❌ Multiple reallocations
vector<int> v;
for (int i = 0; i < 1000; i++) {
    v.push_back(i);
}

// ✅ Pre-allocate
vector<int> v;
v.reserve(1000);
for (int i = 0; i < 1000; i++) {
    v.push_back(i);
}

reserve avoids the geometric reallocation-and-copy pattern that push_back triggers as a vector grows, which matters when the final size is known or reasonably estimable ahead of time. It’s a genuinely cheap, low-risk optimization, which is exactly why it’s easy to over-apply — reserving a size that turns out to be wrong wastes memory instead of saving time, so this is worth flagging only when the loop bound is a real, stable quantity rather than a guess.

Use Appropriate Data Structures

// ❌ Slow (O(n) search)
vector<int> v;
find(v.begin(), v.end(), x);

// ✅ Faster (O(log n) or O(1))
set<int> s;
s.find(x);

unordered_set<int> us;
us.find(x);

This is the one performance item worth blocking a PR over, because it’s an algorithmic complexity change, not a constant-factor one — a linear search inside another loop turns an O(n) operation into O(n²), and that difference is invisible in a diff review unless the reviewer explicitly asks “what does this container’s access pattern look like at the call site, and how does it scale.” A vector is still the right default for small or rarely-searched collections because of cache locality, so the fix isn’t “always use set” — it’s “know the access pattern before picking the container.”

String Concatenation

// ❌ Slow
string result;
for (const auto& s : strings) {
    result += s;  // Reallocation every time
}

// ✅ Faster
ostringstream oss;
for (const auto& s : strings) {
    oss << s;
}
string result = oss.str();

In practice, std::string::operator+= has amortized-constant-time reallocation behavior similar to vector::push_back, so this specific example matters less than it looks — the real win from reserve-ing the string ahead of time (or using operator+= with a known final size) is often comparable to switching to a stream. What’s worth reviewing here is whether string building happens inside a loop that runs per-request in a hot path; if it’s a one-time startup cost, the difference is noise and not worth a review comment.

Move Semantics

// ❌ Copy
vector<int> v1 = getData();
vector<int> v2 = v1;  // Copy

// ✅ Move
vector<int> v1 = getData();
vector<int> v2 = move(v1);  // Move

The review trap here runs in both directions. Missing a std::move on a large object that’s about to go out of scope anyway is a real, measurable cost. But std::move-ing an object and then reading from it afterward is a bug, not an optimization — a moved-from std::vector is guaranteed to be in a valid-but-unspecified state, and any subsequent read (other than reassignment or destruction) is undefined by convention even where the standard doesn’t forbid it outright. A reviewer who sees move(v1) should always check whether v1 is used again below that line.

Readability (5 Items)

Readability checks look like style nits, but several of them are load-bearing for correctness, not just aesthetics — const-correctness and magic numbers in particular affect what the compiler can verify for you, not just what a human finds pleasant to read.

Clear Variable Names

// ❌ Unclear
int d;  // What does this mean?
int tmp;

// ✅ Clear
int daysUntilExpiry;
int userCount;

Naming review matters more in C++ than in a dynamically typed language because C++‘s type alone often doesn’t tell you units, ownership, or valid range — an int could be a count, an index, a duration in milliseconds, or an error code, and the name is frequently the only signal a reader gets. This is a genuine nit-level item, not a blocker, unless the ambiguity is severe enough to plausibly cause a bug (e.g., an unlabeled duration where the unit — seconds vs. milliseconds — is exactly the kind of mismatch that causes real production incidents).

Function Length

// ❌ Too long (100+ lines)
void processData() {
    // ... 100 lines ...
}

// ✅ Split into smaller functions
void validateData() { /* ... */ }
void transformData() { /* ... */ }
void saveData() { /* ... */ }

void processData() {
    validateData();
    transformData();
    saveData();
}

Function length is a proxy for a real problem, not the problem itself: long functions are harder to reason about because they hold more state and more control-flow paths in the reader’s head at once, which is precisely the condition under which lifetime and ownership bugs get missed. A 100-line function with five early returns and three nested try/catch blocks is much more likely to leak a resource on one specific path than three 30-line functions with the same total logic, because each smaller function has a smaller, more auditable set of exit paths.

Remove Magic Numbers

// ❌ Magic number
if (status == 2) {  // What is 2?
    // ...
}

// ✅ Use constants
const int STATUS_ACTIVE = 2;
if (status == STATUS_ACTIVE) {
    // ...
}

// ✅ Use enums
enum class Status { Inactive, Pending, Active };
if (status == Status::Active) {
    // ...
}

The enum class version isn’t just more readable than the raw integer constant — it changes what the compiler can catch for you. Status::Active can’t be implicitly compared against an unrelated integer, can’t be silently passed where a different enum was expected, and gives you an exhaustiveness warning on switch statements that a bare int never will. This is a case where the “readability” fix is really a type-safety fix wearing a readability label.

Comments

// ❌ Unnecessary comments
int x = 10;  // Assign 10 to x

// ❌ Outdated comments
// TODO: Fix this later (2020)

// ✅ Explain intent
// Set timeout to 10 seconds (considering server response time)
int timeout = 10;

A comment that restates the code adds review noise without adding information — worse, it gives false confidence that the line has been explained when it hasn’t. The comments actually worth requesting in review are the ones that capture why, especially the ownership and lifetime assumptions raw pointers can’t express in their own type (see the ownership-transfer section above) — those are exactly the comments that go stale, so a reviewer should also periodically ask whether an existing “safe because X” comment is still true given the current diff.

Const Correctness

// ❌ No const
void print(vector<int>& v) {
    for (int x : v) {
        cout << x << " ";
    }
}

// ✅ Add const
void print(const vector<int>& v) {
    for (int x : v) {
        cout << x << " ";
    }
}

Missing const is more than a style nit — it’s a lost compiler-enforced contract. A const vector<int>& parameter tells every caller, and every future maintainer of the function body, that the vector will not be modified, and the compiler will reject any accidental mutation as a hard error. Drop the const and that guarantee disappears: a caller now has to read the entire function body (and everything it calls) to be sure their data survives the call unmodified, and a future edit that adds a mutation won’t trigger any warning at the call site. This is the difference between an invariant the type system enforces for free and an invariant that exists only as long as everyone remembers to preserve it by hand — which is to say, not for very long. const-correctness review is cheap for the reviewer and expensive to skip, because a missing const compounds: every function that takes the now-non-const reference has to be re-audited the same way.

Safety (5 Items)

This is the category where “the code compiles and the tests pass” is the least reliable signal, because exception safety and thread safety are properties of how code behaves under conditions the happy-path tests don’t exercise — an exception thrown mid-function, or two threads interleaved in a specific order. Neither shows up by reading the code sequentially, which is exactly how a diff gets reviewed.

Exception Safety

// ❌ Leak on exception
void bad() {
    int* ptr = new int[100];
    riskyOperation();  // May throw exception
    delete[] ptr;  // Not executed!
}

// ✅ RAII
void good() {
    vector<int> v(100);
    riskyOperation();  // Safe even if exception occurs
}

Most C++ code review never asks which exception-safety guarantee a function provides — basic (invariants hold, no leaks, but state may have changed), strong (operation either fully succeeds or has no visible effect, like a transaction), or nothrow — and that’s a real gap, especially for RAII-heavy code where destructors run during stack unwinding. The question worth asking on any function that can throw partway through a multi-step operation is: if this throws after step 2 of 4, is the object left in a valid, usable state, or in some half-mutated state that the caller has no way to detect? A function that pushes to one container, then a second operation throws, and the two containers are now out of sync, is a basic-safety violation that no test written for the happy path will ever catch — you need a test that specifically injects a failure at that exact point, which almost nobody writes without being told to.

nullptr Check

// ❌ No check
void process(int* ptr) {
    *ptr = 10;  // What if ptr is nullptr?
}

// ✅ Add check
void process(int* ptr) {
    if (!ptr) {
        throw invalid_argument("ptr is null");
    }
    *ptr = 10;
}

The review question isn’t just “did they check for null,” it’s “should null even be a representable state here.” If a pointer parameter should never legally be null, the better fix is often a reference (int&) or gsl::not_null<int*>, which makes the invalid state unrepresentable instead of catchable-after-the-fact. A null check is the right answer when null is a legitimate, expected input (an optional out-parameter, say); it’s a workaround when it’s papering over an API that shouldn’t have accepted a nullable pointer in the first place.

Integer Overflow

// ❌ Possible overflow
int a = INT_MAX;
int b = a + 1;  // Overflow!

// ✅ Add check
if (a > INT_MAX - 1) {
    throw overflow_error("overflow");
}
int b = a + 1;

Signed integer overflow in C++ is undefined behavior, not wraparound — the compiler is legally permitted to assume it never happens, which means an optimizer can and sometimes does eliminate an overflow check that comes after the overflowing arithmetic, on the theory that if the addition didn’t overflow (because overflow is impossible per the language rules), the check is dead code. This is worth flagging specifically on any arithmetic involving sizes, indices, or lengths derived from untrusted input — a buffer-size calculation like count * sizeof(T) is a classic place for this to turn into a heap overflow if count is attacker-controlled and the multiplication overflows into a small number.

Input Validation

// ❌ No validation
void setAge(int age) {
    this->age = age;  // Negative values allowed?
}

// ✅ Add validation
void setAge(int age) {
    if (age < 0 || age > 150) {
        throw invalid_argument("invalid age");
    }
    this->age = age;
}

The review-worthy question is where the trust boundary actually is. Validating every internal setter defensively adds noise without adding safety if the value already passed validation three call frames up; the check that matters is the one at the boundary where untrusted data — user input, network payloads, file contents — first enters the system. A reviewer should be able to point at the specific boundary a given validation is defending, and if they can’t, the validation is probably either redundant or (worse) missing from the place it’s actually needed.

Thread Safety

// ❌ Race condition
int counter = 0;

void increment() {
    counter++;  // Not thread-safe!
}

// ✅ Use mutex
mutex mtx;
int counter = 0;

void increment() {
    lock_guard<mutex> lock(mtx);
    counter++;
}

Here’s the fact that makes concurrency review structurally different from every other item on this list: a data race is a property of two or more threads’ execution interleaving, and nothing about that is visible by reading a single function’s source top to bottom. counter++ reads as three sequential lines in the diff — load, increment, store — and there’s nothing in that diff that tells you another thread might interleave between the load and the store. You can review this exact code a hundred times, understand every line perfectly, and still miss the race, because the bug isn’t in the code’s logic, it’s in the scheduling the code doesn’t control. This is why I treat any shared mutable state touched from more than one thread as requiring an explicit, separate question in review — “what guards this specific piece of state, and is that guard held on every access, including error paths” — rather than trusting that a function which reads correctly top-to-bottom is therefore correct.

A first-person story: the data race a “sequential-looking” diff hid

The closest I’ve come to shipping a serious concurrency bug was a PR that added a background flush thread to write cached metrics to disk periodically, while the existing request-handling threads kept updating the same cache. The diff, read in isolation, looked completely sequential: acquire the cache, iterate it, write it out, done — nothing about the flush function itself was wrong. What made it a race was something the diff didn’t show at all: the existing code that mutated the same cache from request-handling threads had never needed a lock before, because until this PR, only one thread ever touched it. Adding the second thread was correct in the new file and turned every unguarded read in the old, untouched file into a data race — and because those old call sites weren’t part of the diff, nobody re-reviewed them. We found it via a flaky test that failed roughly one run in thirty under -fsanitize=thread, not by reading the code, because reading the code — even carefully — cannot reveal a race that depends on two files’ independent history intersecting for the first time. The takeaway I apply now: any PR that introduces a new thread accessing existing shared state is a mandatory trigger to re-audit every existing access to that state, not just the new code, because “the diff is small” says nothing about how large the blast radius of an invariant violation is.

Why small diffs catch more lifetime and ownership bugs than large ones

There’s a mechanical reason large PRs are worse for catching the bugs in this checklist specifically, and it’s not just “reviewers get tired.” Ownership and lifetime bugs are relational — the bug isn’t in any single line, it’s in the relationship between where an object is created, where a pointer or reference to it is stored, and where it’s eventually used or destroyed. A human reviewer holds a working set of maybe a few hundred lines of context at a time; once a PR exceeds that, the reviewer necessarily starts reviewing sections independently rather than tracking a pointer’s lifetime across the whole diff, which is exactly the condition under which an ownership bug that spans “created on line 40, stored on line 90, used-after-free on line 310” slips through, because no single review pass held all three lines in context simultaneously. A twenty-line diff that changes one function’s raw pointer to a unique_ptr is auditable in one pass. A eight-hundred-line PR that refactors a subsystem and happens to change the same pointer’s ownership semantics along the way is not — and the failure isn’t the reviewer’s competence, it’s the format. This is the practical argument for insisting on small, incremental PRs specifically for ownership-sensitive code, independent of any general “small PRs are nice” style preference: it’s not about readability, it’s about whether the bugs this checklist exists to catch are even structurally visible to a reviewer at that diff size.

sequenceDiagram
    participant Dev as Developer
    participant PR as Pull Request
    participant Rev as Reviewer
    Dev->>PR: Small diff (one ownership change)
    PR->>Rev: Full context fits in one pass
    Rev-->>Dev: Traces create -> store -> use -> destroy
    Note over Rev: Ownership bug caught
    Dev->>PR: Large diff (subsystem refactor)
    PR->>Rev: Context exceeds working memory
    Rev-->>Dev: Reviews sections independently
    Note over Rev: Ownership bug spans sections,\nnever tracked end to end

Code Review Process

Automated Checks

# Compile warnings
g++ -Wall -Wextra -Werror

# Static analysis
cppcheck --enable=all .
clang-tidy *.cpp

# Format check
clang-format -i *.cpp

Automated checks exist to free up human review time for exactly the things tools can’t see — ownership contracts, exception-safety guarantees, and concurrency invariants. A team that runs clang-tidy and cppcheck in CI but still spends review time flagging formatting or naming issues those tools already catch is wasting the one resource code review can’t scale: a human’s attention. Treat a red static-analysis run as a hard gate before a human even opens the diff.

Manual Checks

□ Does the code meet requirements?
□ Are tests sufficient?
□ Is error handling appropriate?
□ Are there performance issues?
□ Are there security vulnerabilities?
□ Is the code readable?
□ Is it documented?

This list is a starting checklist, not a complete one — notice it doesn’t explicitly say “trace every raw pointer’s ownership” or “does this PR touch shared mutable state,” which, per the sections above, are the two questions most likely to catch a real bug. A team’s actual review checklist should be built from its own postmortems: whatever category of bug has escaped to production twice belongs on the list explicitly, worded as a question, not as a vague reminder to “be careful.”

Writing Feedback

✅ Good feedback:
"line 42: Using const reference for the vector can avoid copying."

❌ Bad feedback:
"This code is terrible."

The difference between these two isn’t just tone — the good version is falsifiable and actionable (the author can look at line 42 and agree or push back with a specific reason), while the bad version gives the author nothing to act on and actively discourages the kind of open “wait, why did we do it this way” conversation that surfaces the ownership and lifetime questions this whole post is about. Reviewers who feel safe asking “why does this raw pointer not own the object” without it reading as an accusation get better answers than reviewers who don’t.

Practical Example

Example 1: Code Before Review

void process(vector<int> data) {
    int* arr = new int[data.size()];
    
    for (int i = 0; i <= data.size(); i++) {
        arr[i] = data[i] * 2;
    }
    
    // ... processing ...
    
    delete arr;  // Not delete[]!
}

Issues:

1. Unnecessary copy (vector) — data is passed by value and never mutated, so a const& avoids copying the whole container on every call.
2. Use of raw pointer — the new[]/delete pair has to be manually matched, and it already isn’t (see issue 4), which is exactly the kind of mismatch RAII eliminates.
3. Out-of-bounds access (i <= data.size()) — the loop condition should be <, not <=; as written this always writes one element past the end of both arr and reads one past the end of data, and it’s undefined behavior on every single call, not just an edge case.
4. delete vs delete[] — using scalar delete on an array allocated with new[] is undefined behavior; the destructor and deallocation bookkeeping for arrays differ from single objects, and mismatching them can corrupt the heap.

Example 2: Code After Review

void process(const vector<int>& data) {
    vector<int> result;
    result.reserve(data.size());
    
    for (int value : data) {
        result.push_back(value * 2);
    }
    
    // ... processing ...
}

Improvements:

1. Removed copy by using const reference — the caller’s vector is no longer duplicated just to read it.
2. Used vector (RAII) — no manual new/delete pairing to get wrong, and no leak on any exception path.
3. Range-based for loop — the off-by-one in the original loop bound is structurally impossible here; there’s no index to get wrong.
4. Improved performance with reserve — one allocation instead of the geometric reallocation push_back would otherwise trigger.

Notice all four of the original bugs are fixed by two changes — pass by const&, allocate with a container instead of raw new[] — which is a useful pattern to recognize in review: a surprising number of unrelated-looking issues (a copy, a leak, an off-by-one, a mismatched deallocator) often trace back to a single root cause, in this case “raw pointer arithmetic where a standard container would have been safer and shorter.”

FAQ

Q1: How often should code reviews happen?

A: Ideally, for every PR/commit — and the smaller and more frequent, the better a reviewer’s chance of catching ownership and lifetime bugs, per the discussion above on why large diffs hide relational bugs.

Q2: How many reviewers are needed?

A: At least 1, but 2 or more for code that touches shared mutable state, ownership semantics, or anything on a security boundary — the categories where a single reviewer’s blind spot is most costly.

Q3: How long should a review take?

A: Around 1 hour for 200-400 lines of code is a reasonable ceiling, not a target — past that size, review quality drops sharply because the diff exceeds what a person can hold in working memory at once, which is the same mechanism discussed above for why large PRs let ownership bugs through.

Q4: What tools can be used for automation?

A:

• clang-tidy (catches many ownership and lifetime patterns, e.g. bugprone-use-after-move)
• cppcheck
• SonarQube
• Coverity
• Sanitizers (-fsanitize=address, -fsanitize=thread) run in CI, not just locally — these catch exactly the memory and concurrency bugs a static diff review structurally cannot

Q5: What is a good code review culture?

A:

• Provide constructive feedback
• Focus on improving code, not criticizing the author
• Treat it as a learning opportunity
• Make it normal to ask “what happens to this pointer’s ownership if X changes” without it reading as an accusation

Q6: How to create a review checklist?

A:

1. Analyze past bugs in the team
2. Identify common mistakes
3. Include coding style guidelines
4. Word each item as a question about an invariant (“who owns this after the call,” “what guards this shared state”), not just a style preference — the goal is to force the reviewer to check the specific thing that has previously escaped to production.

Related Articles

• C++ RAII Explained | Why Constructors and Destructors Prevent Leaks
• C++ const Correctness: Practical Guide
• C++ Smart Pointers