Skip to content

Code Review Process

Overview

Code review ensures quality, knowledge sharing, and catches issues before merge.

Roles

Author Responsibilities

  • Self-review before requesting review
  • Ensure CI passes
  • Provide clear PR description
  • Respond to feedback promptly
  • Be open to suggestions

Reviewer Responsibilities

  • Review within 24-48 hours
  • Provide constructive feedback
  • Test locally if needed
  • Approve only if confident
  • Explain reasoning for changes requested

Review Checklist

Functionality

  • Code does what PR description claims
  • Edge cases handled
  • Error handling appropriate
  • No obvious bugs

Design

  • Follows existing patterns
  • Appropriate abstraction level
  • No over-engineering
  • API is intuitive

RT Safety (Audio Thread Code)

  • No locks in RT path
  • No allocations in RT path
  • No syscalls in RT path
  • Lock-free structures used correctly
  • Wait-free where possible

Code Quality

  • Follows coding standards
  • Clear variable/function names
  • Appropriate comments
  • No commented-out code
  • No debug print statements

Testing

  • Tests cover new code
  • Tests are clear and maintainable
  • Edge cases tested
  • RT safety validated

Performance

  • No obvious performance issues
  • Appropriate data structures
  • No unnecessary copies
  • Cache-friendly access patterns

Cross-Platform

  • Windows compatible
  • Linux compatible
  • macOS compatible
  • No platform-specific assumptions

Documentation

  • Public APIs documented
  • Complex logic explained
  • README updated if needed
  • Examples provided if API changed

Review Priorities

P0: Must Fix (Block Merge)

  • Correctness issues
  • RT safety violations
  • Security vulnerabilities
  • Breaking changes without migration path
  • Test failures

P1: Should Fix (Request Changes)

  • Design issues
  • Performance problems
  • Missing tests
  • Poor error handling
  • Code style violations

P2: Nice to Have (Comment Only)

  • Suggestions for improvement
  • Alternative approaches
  • Future enhancements
  • Nitpicks

Feedback Guidelines

Constructive Comments

✅ Good: 

This could lead to a race condition if called from multiple threads.
Consider using atomic operations or document thread-safety requirements.

❌ Bad: 

This is wrong.

Ask Questions

✅ Good: 

Why did you choose std::vector over std::array here?
Is the dynamic sizing needed?

Suggest Alternatives

✅ Good: 

Have you considered using a ring buffer here?
It might be more cache-friendly for this access pattern.

Praise Good Code

Nice use of constexpr here - this will be optimized away at compile time.

Review Workflow

1. Initial Review

# Pull PR branch
git fetch origin pull/ID/head:pr-ID
git checkout pr-ID

# Build and test
cmake --build build
ctest --test-dir build

2. Provide Feedback

  • Use GitHub review comments
  • Group related comments
  • Suggest specific changes
  • Link to relevant documentation

3. Re-Review

  • Check if feedback addressed
  • Verify changes make sense
  • Test again if major changes

4. Approval

  • Explicitly approve in GitHub
  • Add comment summarizing review
  • Merge or let author merge

Common Review Scenarios

Large PRs

This is a lot to review. Consider breaking into smaller PRs:
1. Core data structure
2. Algorithm implementation
3. Tests and benchmarks

Missing Tests

Could you add a test for the error case when buffer is null?
Example:
TEST_CASE("ProcessBuffer handles null buffer") {
    REQUIRE_THROWS(processBuffer(nullptr));
}

RT Safety Issue

This allocates in the audio callback (line 42):
    buffer.resize(newSize);  // ❌ RT unsafe

Consider pre-allocating to max size:
    buffer.reserve(MAX_SIZE);  // ✅ RT safe

Performance Concern

This iterates the vector twice (lines 10, 15).
Could combine into single pass:
for (auto& item : items) {
    transform(item);
    filter(item);
}

Time Expectations

Review Times

  • Small PR (<100 lines): 15-30 minutes
  • Medium PR (100-300 lines): 30-60 minutes
  • Large PR (>300 lines): 1-2 hours

Response Times

  • Author should respond to feedback: within 1-2 days
  • Reviewer should review: within 1-2 days
  • Critical fixes: same day

Review Tools

GitHub Features

  • Line comments for specific issues
  • Review summary for general feedback
  • Suggestion blocks for code changes 
    // Suggested change
const auto result = compute();

Local Testing

# Run specific tests
ctest -R TestName

# Run with sanitizers
cmake -DENABLE_ASAN=ON ..
cmake --build .
ctest

# Check formatting
clang-format --dry-run --Werror src/**/*.cpp

Anti-Patterns to Avoid

Rubber Stamping

❌ Don't approve without actually reviewing ✅ Take time to understand the changes

Perfectionism

❌ Don't block on minor style issues ✅ Focus on correctness and design

Personal Preferences

❌ "I would have done it differently" ✅ "This approach has performance implications"

Scope Creep

❌ "While you're here, also refactor X" ✅ Keep feedback focused on PR scope

Handling Disagreements

  1. Explain reasoning: Share why you think something matters
  2. Seek understanding: Ask author's perspective
  3. Find common ground: What's the real goal?
  4. Escalate if needed: Get third opinion
  5. Be willing to defer: Some things can be future PRs

Knowledge Sharing

Use reviews to: - Share domain knowledge - Teach best practices - Learn new techniques - Build team cohesion

Example: 

TIL about std::launder for this use case!
I've been using reinterpret_cast incorrectly.

Review Metrics (Optional)

Track (but don't optimize): - Average review time - Bugs caught in review - Post-merge bugs - Review comments per PR

Goal: Improve process, not judge people.