What I look for when reviewing C++ code
The things I actually check in a C++ PR — ownership, lifetime, and the stuff that bites you six months later.
After a few years of C++ code review, I’ve developed a short mental checklist. Not an exhaustive style guide — those are mostly noise. This is the stuff that actually catches real bugs.
Ownership and lifetime
This is the big one. Who owns this object? How long does it live? Does this pointer stay valid?
If I can’t answer these in under thirty seconds of reading, that’s a sign the code needs clarification — either through naming, a comment, or restructuring. shared_ptr everywhere is not the answer; it’s the code saying “I gave up thinking about this.”
Questions I ask:
- Who deletes this?
- Is this a raw observer pointer? Is that documented?
- Are there any dangling reference possibilities here — especially in lambdas or async callbacks?
Copy vs. move semantics
You’d be surprised how often a std::string gets copied where a reference or move would do. Not always a bug, but often a performance problem in hot paths.
I look for:
- Functions taking
std::string(or other containers) by value when they don’t need to own a copy - Missing
std::movewhen transferring ownership - Unnecessary copies of large objects inside loops
Undefined behavior magnets
The usual suspects: signed integer overflow, out-of-bounds access, using a moved-from object, uninitialized reads.
I don’t catch all of these by eye — sanitizers exist for a reason. But I look for patterns that commonly produce UB even when they “work” today.
Error handling
What happens when this fails? Is the failure mode silent? Does the caller know it can fail?
In game code this often looks like returning nullptr and hoping the caller checks. Sometimes that’s fine; sometimes it produces a subtle stall or a later crash that’s hard to attribute.
This is a living document. I update it when I find a new category that keeps appearing in review.