Skip to content

chore: Address the top 3 clang-tidy warnings by count - #634

Merged
beekld merged 4 commits into
mainfrom
bklimt/clang-tidy-optional-access
Oct 26, 2026
Merged

beekld merged 4 commits into
mainfrom
bklimt/clang-tidy-optional-access

Conversation

@beekld

@beekld beekld commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Addresses the top 3 clang-tidy warnings by count.

Moves the clang-tidy check list from the cpp-linter workflow into .clang-tidy, so all lint configuration lives in one file and per-directory overrides work.

  • bugprone-unchecked-optional-access (509 warnings): Disabled, because every instance in production was a false positive.
    • 321 were trivially provable false positives, because the same function had already checked the value.
    • 35 were guaranteed by other code in the same class.
    • 14, all in tests, were guaranteed by the documented behavior of Future.
    • 136, all in tests, were never checked, so a regression would crash the test instead of failing it.
    • 3 relied on code outside the class and were fragile, so this PR makes those more robust.
  • cppcoreguidelines-avoid-magic-numbers and readability-magic-numbers (495 warnings, the same finding under two names): Disabled, because the numbers it flags are already clear in context.
    • 448 were test data.
    • 26 defined default values, so the function or member that holds each one already names it.
    • 17 were HTTP status codes.
    • 2 were self-explanatory, a hex base of 16 and Beast's 11 for HTTP/1.1.
    • 2 were the same 5-minute read timeout, hardcoded in the client and server FDv1 streaming data sources.
  • readability-identifier-length (372 warnings): Disabled, because short names are idiomatic where it flags them, and reviewers can judge the rest case by case.
    • 284 were in tests.
    • 63 were one-letter names in production code, such as a and b in comparison operators, or f for a callable.
    • 25 were two-letter names in production code, such as op for a clause operator or ms for milliseconds.
    • We could add these names to the existing ignore lists in .clang-tidy, but that doesn't seem worth the effort.

Note

Overview
Centralizes clang-tidy configuration in .clang-tidy (full check set plus disables for bugprone-unchecked-optional-access, magic-number rules, and readability-identifier-length) and sets the cpp-linter workflow’s tidy-checks to empty so CI uses that file and per-directory overrides work.

Runtime hardening replaces unchecked optional::value() on FDv2 server errors and polling bodies: FDv2 streaming/polling paths now guard server_error before reading payload ids, and client polling treats HTTP 200 with no body as interrupted instead of dereferencing the body. Evaluation refactors MakeBigSegmentRef to take segment key and generation after the generation null-check, avoiding optional access through the whole Segment.

Reviewed by Cursor Bugbot for commit a7b4fb1. Bugbot is set up for automated code reviews on this repo. Configure here.

@beekld
beekld marked this pull request as ready for review October 26, 2026 19:27
@beekld
beekld requested a review from a team as a code owner October 26, 2026 19:27
@beekld
beekld merged commit 915b47f into main Oct 26, 2026
55 checks passed
@beekld
beekld deleted the bklimt/clang-tidy-optional-access branch October 26, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants