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.
- 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.
Pointer and Value Checks
- Explicit
nullptr Comparisons: Always compare pointer types to nullptr explicitly. Avoid implicit boolean conversion in conditions.
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.
- Encapsulation: Make classes' data members private unless they are constants.
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.
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.
Operator Style
- Pre-increment/decrement: Prefer the prefix form (
++i, --i) unless postfix semantics are required.
Smart Pointer Initialization
- Assignment Syntax: Use assignment syntax when initializing directly with smart pointers.
Modern C++ Patterns (C++20 & Standard Library)
- Constinit: Use
constinit directly instead of ABSL_CONST_INIT. - std::exchange: Use
std::exchange to simplify reset-and-return logic. - 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.
Type Casting and Conversions
- Prefer Braced Initialization for Non-Narrowing Conversions: Prefer braced initialization (
Type{value}) over static_cast<Type>(value) when converting constants or values where narrowing is not intended.- Correct:
if (temporal_index >= int{kMaxTemporalStreams}) - Incorrect:
if (temporal_index >= static_cast<int>(kMaxTemporalStreams)) - Reference: Google C++ Style Guide - Casting
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. - 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.
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). - 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. - Descriptive Test Names: Include the test scenario and expected outcome in test names.
- Do Not Crash in Tests: Use
FAIL() << "message"; instead of triggering a crash when verifying preconditions or expectations. - Avoid Double Negations: Use positive assertions for better readability.
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.
- Keep CLs Small: Split large changes into multiple CLs. Introduce interface definitions in one CL, and migrate callers in subsequent CLs.
- Documentation Freshness: When adding new
.md files, always include freshness metadata to track ownership.
5. Inclusive Language Guidelines
- Use Inclusive Terminology: Avoid gendered pronouns and non-inclusive terms in code, comments, and documentation. Use precise, neutral technical alternatives.