pre-reviewer

name	pre-reviewer
description	Automated pre-review tool to inspect C++ style, concurrency safety, test quality, and CL descriptions before submitting code for human review.

Pre-Reviewer Skill

This skill acts as a pre-review confidence check tool for WebRTC C++ contributions. Use this skill to evaluate code changes for common C++ anti-patterns, concurrency bugs, test hygiene issues, and CL description completeness.

---

1. C++ Style and Modernization Guardrails

Lambda Expressions

• No Capture Renaming: Do not use captures with initializers to introduce new names. Capture variables with their original names.
  • Correct: [on_certificate_ready = std::move(on_certificate_ready)]
  • Incorrect: [callback = std::move(on_certificate_ready)]
  • Reference: Google C++ Style Guide - Lambda expressions
• Prefer Implicit Captures for Inline/Local Lambdas: If the lambda cannot escape the current scope (e.g. executed inside BlockingCall), prefer auto-capture ([&]). Do not explicitly list captures as it reduces readability.
  • Reference: C++ Readability Advice - Lambda capture clauses

Pointer and Value Checks

• Explicit nullptr Comparisons: Always compare pointer types to nullptr explicitly. Avoid implicit boolean conversion in conditions.
  • Correct: if (tq_ != nullptr) / RTC_DCHECK(transport == nullptr);
  • Incorrect: if (tq_) / RTC_DCHECK(!transport);
  • Reference: Abseil Tip #141: Test Container Emptiness with Boolean Functions

Structs vs. Classes

• Classes for API Invariants: Use a class if the object has invariants, has wide visibility, or is expected to evolve. Use a struct only for passive data.
  • Reference: Google C++ Style Guide - Structs vs. Classes
• Encapsulation: Make classes' data members private unless they are constants.
  • Reference: Google C++ Style Guide - Access Control

Virtual Functions

• No Default Arguments on Virtuals: Banned on virtual functions because default arguments are bound statically at compile time, which leads to unexpected behavior when overriding.
  • Reference: Google C++ Style Guide - Default Arguments

Type Deduction (auto)

• Avoid Excessive auto: Explicitly spell out types unless the type is overly verbose (e.g. templates) or using auto directly improves safety. Do not use it merely to avoid writing the type.
  • Reference: Google C++ Style Guide - Type deduction

Operator Style

• Pre-increment/decrement: Prefer the prefix form (++i, --i) unless postfix semantics are required.
  • Reference: Google C++ Style Guide - Preincrement and Predecrement

Smart Pointer Initialization

• Assignment Syntax: Use assignment syntax when initializing directly with smart pointers.
  • Correct: std::unique_ptr<Foo> foo = std::make_unique<Foo>();
  • Incorrect: std::unique_ptr<Foo> foo(std::make_unique<Foo>());
  • Reference: Abseil Tip #88: Use Assignment Syntax for Smart Pointer Initialization

Modern C++ Patterns (C++20 & Standard Library)

• Constinit: Use constinit directly instead of ABSL_CONST_INIT.
  • Reference: cppreference - constinit
• std::exchange: Use std::exchange to simplify reset-and-return logic.
  • Reference: cppreference - std::exchange
• Avoid std::make_pair / std::make_tuple: Prefer brace initialization or class template argument deduction (CTAD) like std::pair{a, b} instead of std::make_pair(a, b).
• absl::flat_hash_map: Prefer as the default map type unless ordering is required.
  • Reference: Abseil Container Guide

Move Semantics

• Move First, Dereference After (for std::optional): For std::optional, it is recommended to move first and dereference after (e.g. *std::move(opt)). Note that this is generally not recommended for std::unique_ptr.
  • Reference: Abseil Tip #181: Move First, Dereference After
  • Reference: Abseil Tip #224: Avoid value() accessors
• Avoid Redundant Return moves: Do not use std::move on return statements when it prevents copy elision (NRVO/RVO). Moving a member variable out of a class when returning can still be useful.
• Avoid Brittle Move Loops: Avoid moving a member variable into an argument of a method only to move it back inside the method. This looks like a "use after move" and makes code brittle.
  • Example: Avoid RecreateEncoder(std::move(config_)) which internally updates config_ back. Prefer separating it into a parameterless reset method or a config updater.

---

2. Concurrency, Threading, and Destructors

• Prevent Reentrancy in Destructors: When executing callbacks during destruction, move or swap the callback list/queue into a local variable first, then execute. This prevents callbacks from accessing a partially destroyed class.
• No Sequence Checkers in Destructors: Remove or omit sequence/thread checks (RTC_DCHECK_RUN_ON) inside destructors. In C++, it is assumed that no other threads can call into an object while it is being destroyed.
• Atomic Safety: Do not use relaxed memory order atomics when safety invariants depend on the ordering of operations not protected by the same lock.
  • Reference: Abseil - Atomic Danger

---

3. Testing Best Practices

• Omit GMock Default Cardinality: Do not add .Times(1) to EXPECT_CALL as it is the default.
• Omit Wildcard GMock Parameters: Omit parameter lists in EXPECT_CALL when all parameters are wildcards (applicable to non-overloaded methods).
  • Reference: GoogleTest Mocking Reference - EXPECT_CALL
• Avoid Unrelated Assertions: Do not assert expectations on calls that are not the focus of the test scenario. Use ON_CALL instead of EXPECT_CALL for setting default behavior without setting expectations.
  • Reference: GoogleTest Cookbook - Use ON_CALL
• Descriptive Test Names: Include the test scenario and expected outcome in test names.
  • Reference: Testing on the Toilet: Writing Descriptive Test Names
• Do Not Crash in Tests: Use FAIL() << "message"; instead of triggering a crash when verifying preconditions or expectations.
  • Reference: GoogleTest Reference: Success/Failure Assertions
• Avoid Double Negations: Use positive assertions for better readability.
  • Reference: Testing Blog: Improve Readability with Positive Assertions

---

4. Code Review and CL Process

• Meaningful CL Descriptions: The CL description must explain why the change is made, not just what changed. List major goals, link issue tickets, and clarify the bigger architectural plan.
  • Reference: Google Engineering Practices: CL Descriptions
• Keep CLs Small: Split large changes into multiple CLs. Introduce interface definitions in one CL, and migrate callers in subsequent CLs.
  • Reference: Google Engineering Practices: Small CLs
• Documentation Freshness: When adding new .md files, always include freshness metadata to track ownership.
  • Reference: WebRTC Documentation Guidelines

---

5. Inclusive Language Guidelines

• Use Inclusive Terminology: Avoid gendered pronouns and non-inclusive terms in code, comments, and documentation. Use precise, neutral technical alternatives.
  • Reference: Inclusive Chromium Code
  • Reference: Google Developer Documentation Style Guide - Word List