fboss-code-standards

# FBOSS Code Standards ## Overview Passive coding guidance for FBOSS. Applied automatically while writing or modifying code under `fboss/`. ## Scope Currently `fboss/` only. TODO: extend to `configerator/source/neteng/fboss`, `neteng/netcastle`, `neteng/fboss`. ## Quick Checklist | Area | Pattern | Check | |------|---------|-------| | Agent | Mono/multi-switch duality | State changes must work in both modes | | Agent | SW/HW build boundary | Production HW-agent executables do not depend on SW-agent implementation libraries | | SAI/Build | Executable symbol exports | Export only needed SDK symbols in `opt`; keep sanitizer exports working | | Agent | Warmboot serialization | New SwitchState fields must serialize/deserialize | | SAI | SaiApiTable registration | New SAI attributes must be registered | | SAI | SaiStore consistency | SAI objects must be tracked, no orphans | | FSDB | State/Stats duality | Always handle both trees | | FSDB | extern template | New ThriftStructNode instantiations need extern template | | thrift_cow | COW modification | Use `modify()`, never mutate shared nodes | | Platform | JSON + Thrift sync | Config changes update both schemas | | Testing | Naming convention | Follow `AgentHw<Feature>Test` pattern | | Testing | NSDB impact | Core FSDB changes must run NSDB tests | | General | Follow local patterns | Be consistent with existing code in the directory | | Testing | No GTEST_SKIP | Use ProductionFeatures filtering, never `GTEST_SKIP()` | | Testing | DSF counters | Check reassembly errors on fabric ports, not discards | | Agent | Non-coalescing behavioral deltas | Mark delta-significant updates non-coalescing | | Agent | Rolled-out flag cleanup | Remove feature flags that are fully rolled out | | Agent | Early return on empty deltas | Check for empty deltas and return early | | Agent | No hardcoded ASIC types | Use feature/property lookups, not ASIC name checks | | SAI | Explicit cancellation | Distinguish reconnectable errors from shutdown signals | | General | No private fn defaults | Don't default parameters all callers override | | General | Explicit state flags | Pass all enable/disable flags, no direction assumptions | | General | Verify before deleting | Provide evidence files are unused before deleting | | General | CHECK over assert | Use CHECK for production invariants, not assert() | | General | emplace over operator[] | Use emplace/insert for map insertions | | General | ASIC feature gating | Gate HW-specific constraints with ASIC features | | General | Concise method naming | Don't include class name in method name | | General | Explicit operator precedence | Parenthesize compound boolean/comparison expressions | | General | Inline trivial comparisons | Don't wrap one-line checks in helper methods | | General | Fix lint before landing | Run `arc f` and `arc lint` before submitting | | Testing | Targeted unit tests | New modules need dedicated unit tests, not just HW tests | | Agent | Documentation-value methods | Keep per-ASIC limit methods as architecture docs | | General | Rate-limit verbose logging | Use XLOG_EVERY_MS, never log at line rate | | General | Perspective-aware naming | Use fromX/toX instead of ambiguous Rx/Tx | | General | Update OSS build files | Update CMake alongside BUCK when adding/moving files | | Testing | Deep comparison over size checks | Compare content, not just collection size | | Testing | Verify negative cases | Explicitly check absence, not just no-throw | | Testing | Test overflow and error paths | Cover resource exhaustion, not just happy path | | Testing | Optimize test execution time | Use smallest packet sizes, avoid unnecessary waits | | Agent | ODS counters for debugging | Add ODS counters for drop/error events, not just logs | | Agent | Validate external config limits | Reject unreasonable externally configurable values | | Agent | Populate warmboot cache first | Fill all caches from warmboot state before processing | | General | Empty vector over optional\<vector\> | Use empty vector, not optional\<vector\> | | General | Map-based config | Use map lookups, not if-else chains for platform values | | General | Log IDs + names | Include both numeric ID and name in log messages | | Agent | Delta processing order | Remove → Change → Add; document in comments | | Agent | Validator extraction | Extract complex validation into dedicated classes | | Agent | Gate validations with flags | New enforcement behind FLAGS for gradual rollout | | Testing | Test transient states | Test intermediate invalid states, not just final | | Testing | Consolidate test helpers | Move shared helpers to base classes | | Testing | Validation UTs | Pure validation → unit tests, not just HW tests | | Testing | No setup changes to existing tests | Create new tests instead; setup changes break warmboot roundtrip CI | | Testing | Program routes via routeUpdater | Use `routeUpdater.program()` (`SwSwitchRouteUpdateWrapper`); never hand-build routes and add to FIB; keep `enable_nexthop_id_manager` on so every route gets a nexthop ID | | Agent | Read nexthops via ID-aware helpers | State/FIB-side code → `FibHelpers` `getNextHops(state,…)` etc.; RIB-internal code → `*FromRib(manager,…)`; never `getNextHopSet()` (inline nexthops are being removed) | ## When to Load References | Topic | Action | |-------|--------| | Team coding conventions (C++ style, control flow, naming) | Read `references/general-patterns.md` | | Agent code (SwSwitch, HwSwitch, mono/multi-switch, warmboot) | Read `references/agent-patterns.md` | | SAI/SDK layer (SaiApi, SaiStore, SaiManager, vendor SDK) | Read `references/sai-sdk-patterns.md` | | FSDB or thrift_cow (subscriptions, COW nodes, PatchBuilder) | Read `references/thrift-cow-fsdb-patterns.md` | | Platform services or config (sensor_service, fan_service, JSON configs) | Read `references/platform-config-patterns.md` | | Tests (agent HW tests, multinode tests, naming, fixtures) | Read `references/testing-patterns.md` | | Adding new patterns | Read `references/contributing.md` |