Make code comments name the hazard and point, with the design living in docs - #303
Merged
Merged
Conversation
arasHi87
added
the
done
…carry the design The comment rules now lead with a single idea: the code is the explanation, and a comment exists to stop a wrong edit and point at the document that owns the reasoning. A ~7-line target replaces the old open-ended 'self-contained comment' convention, and moving prose out becomes a two-step edit — confirm the content is in docs/ first, add it there if not, only then cut. Never delete the only record. Also records the BPF-ABI layering carve-out, which until now existed only as a comment in internal/bpf/abi.go: importing the ABI value types from L2/L3/L4 is a shared data contract, not a layer violation, and must not be 'fixed' by hand-copying generated struct layouts. The C placement rules tighten too. No member of a declaration gets a comment block of its own — C has no doc tool that renders one, so inside a struct body it is only a visual interruption — and member notes are one-line trailing, all-or-none across the declaration. Signed-off-by: arashi.li <arashi.li@bigstack.co>
…AL schema history Both of these existed only as code comments, so the comment tidy-up would have deleted the sole copy of each. The device_owner partition — which owners are infra, which are VM-like, and the network:floatingip carve-out — is verified ground truth on the OVN-Yoga target, including cube:mgr and the trunk-subport case. It now sits under the five-step algorithm, where Step 4 consumes it, with the prefix-match-not-allowlist rationale and the separate IsKnownVMOwner allowlist that only drives the drift warning. The WAL schema history was worse: data-structures.md still described a v3 envelope while wal.SchemaVersion was at 7, so the code comment was the only accurate record. The history is now a table stating why each bump stayed additive and therefore why an older file still loads, plus the note that adding a version means adding a row. One discrepancy surfaced by writing them down: the code comment cited a 37-tenant host at ~14252 pre-dedup entries, the doc a 27-tenant host at 9270. Left as the doc's figures pending a re-measure. Signed-off-by: arashi.li <arashi.li@bigstack.co>
…e comments can go Two operations in handle_packet were carrying multi-line comments that described what a well-named function would have said outright. is_group_mac() replaces a bare 'key.dst_mac[0] & 0x01' and the six lines explaining the IEEE 802 I/G bit. endpoints_from_vm() replaces the three-way ternary swap and its nine lines: the returned struct's fields are vm_mac / peer_mac / remote_ip, which is the whole of what the comment used to spell out. Both are __always_inline, so the generated program is unchanged in shape. What stays on endpoints_from_vm is the part the code still cannot say — that both hooks sit on the same tap, so reading h_source/daddr unconditionally makes egress look the VM's own address up as its peer, and mis-bills silently. The rest of the file follows the tightened C rules: no comment block inside a declaration body (the wire-byte-order note moves above the lpm_key initializer, and all three members take one-line trailing notes), and no block over seven lines outside the file header. Comment share drops from 51% to 43%. Verified with task test-integration — the program loads, so the verifier accepts the struct-returning helper — and task bench-gate-integration, which holds the hot path at zero allocations. Signed-off-by: arashi.li <arashi.li@bigstack.co>
…cs that own it A repo-wide pass applying the ratified rules. Blocks of 25+ lines are gone entirely (29 of them), and the 15-24 band drops from 64 to 5 — the five that remain are package entry points whose length is an invariant list, which the convention allows. The pattern throughout: keep the sentence a reader would otherwise get wrong, drop the paragraph explaining why, point at the document. The worst case was BuildTrie's 89-line transcription of the five-step algorithm that trie-construction.md already owns, now 8. state.go went 169 comment lines to 74. Where a block was the only record of something, the content moved to docs/ first — see the preceding commit. Also fixed on the way: two file-scope headers had lost the blank line before their package clause, so godoc had silently promoted them to package documentation, and internal/scenariotest carried a second 'Package scenariotest' comment alongside its doc.go, which godoc concatenates. Two literal **bold** markers removed — godoc has no bold, so they reached the reader as asterisks. Citations moved to their own trailing line, once per block, never mid-sentence: an anchored path runs 50-80 characters and broke both the sentence and the 80-column wrap. That is why the validated-reference count falls from 321 to 261 — repeats collapsing, not citations lost. Go comment density 30.9% -> 27.0%. task test and task test-integration both green; the docs guard validates every remaining reference. Signed-off-by: arashi.li <arashi.li@bigstack.co>
arasHi87
force-pushed
the
arashi.li/comment-standard
branch
from
August 19, 2026 02:04
718bf09 to
dbe6ae6
Compare
arasHi87
added
done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes
Closes #122
What & why
#122 asked for a comment tidy-up. The scan that opened it overturned three of its four premises, and the convention it recorded got reversed during review — so this lands a different, and I think better, rule than the ticket describes.
The rule. The code is the explanation; a comment exists to stop a wrong edit and point at the document that owns the reasoning. Target ~7 lines. The ticket's convention was the opposite ("self-contained comments, with a trailing pointer as a depth link, not the sole explanation"), which is what produced walls like
BuildTrie's 89-line transcription of an algorithmtrie-construction.mdalready owns.Moving prose out is a two-step edit. Confirm the content is in
docs/first; if it is not, add it there before cutting. Three things turned out to live only in code and are now documented — thedevice_ownerpartition table, the WAL schema history (data-structures.mdstill said v3 while the code was at v7), and the BPF-ABI layering carve-out.Naming beats shortening. The last commit is the version of this that actually works:
is_group_mac()andendpoints_from_vm()delete fifteen lines of comment by giving the operations names, rather than compressing the description of them.Numbers: blocks of 25+ lines 29 → 0; the 15–24 band 64 → 5 (all package entry points carrying invariant lists). Go comment density 30.9% → 27.0%.
bpf/telemetry.c51% → 43%.Test plan
task testtask test-integration(Docker, BPF caps)task bench-gate(hot-path changes only)scenariotest run <name>task test-integration: 40 packages, 0 failures — including the kernel-loading E2E tests, so the verifier accepts the new__always_inlinehelpers.task bench-gate-integration: everyBenchmarkHotpath_still at zero allocations. The docs guard validates every remaining reference.Scope / deliberately not touched
cmd/scenariotest/scenarios/registry.goandhost_route_nexthop.go— untouched throughout, to avoid colliding with in-flight work ondevelop.zone_codeandflow_metricsreference numbers were not re-measured; see Notes.Notes
Contracts. Touches the packet path and the billing-path comments, but no contract behaviour.
bpf/telemetry.cis the only functional change and it is a pure extraction:is_group_mac()andendpoints_from_vm()are__always_inline, the directional swap picks the same MAC and IP per hook as before, and the group-MAC test is the same bit. Contract 5's in-bandcreated_nscomparison, Contract 2's single-lockCollect, and Contract 7's fold-forward are unchanged — only the prose around them moved.One thing for the reviewer to resolve. Writing the two copies side by side surfaced a drift: the
subnet_zone_triecomment cited a 37-tenant host at ~14,252 pre-dedup entries, whiledata-structures.md:109cites a 27-tenant host at 9,270. Both cannot describe the same measurement. I kept the doc's figures rather than pick a winner — if the 37-tenant number is the newer one, the doc needs the update. This is exactly the drift the convention's argument predicts, and it had already happened.Follow-up worth filing. The extraction in the last commit is the pattern the whole ticket wanted. There are likely more sites where a comment is describing code that wants a name —
settleAttributionChange/SettleRebaseand the four-tier fold inCollectare where I would look first.#122's DoD needs rewriting, not ticking. Two of its bullets no longer describe this work: the
doc.go-per-package item (46 of 48 packages already had a proper package comment — it was a file-placement preference, not a gap) and thetelemetry.c"one self-contained file-header model block" item (each model fact appears exactly once, adjacent to the code it guards; consolidating would move explanations away from the line being edited).DoD
/bigstack-core:save-to-handbook)