docs: comments state current behaviour

[thedavidmeister]

Closes #138

Changes

NatSpec and inline comments only, across src/ and test/. No code, test name, assertion or import changes.

Rule applied to every comment: rewrite or drop a sentence whose subject is not in the current tree. That covers a hypothetical fault or mutant ("a guard that did X would…", "…is a different number here", "Delete any of those tests and…"), an alternative not built ("rather than searched for", "Any list would do"), history, a stale quote of replaced NatSpec, and what other tests miss. It also covers a sentence that argues why instead of stating what. Present-tense facts checkable against the code stay.

The crossFunction contract header this sweep rewrote is on main already, byte-identical, from the #121 PR (#185), so it is no longer in this diff.

QA

• Discriminating tests: n/a. The diff changes comments only, so no test can pass or fail differently. forge build compiles, and CI runs the suite unchanged.
• Mutations applied: n/a. No executable line changed, so there is nothing to mutate.
• Oracle: the current tree. Each rewritten sentence is checked against the code it describes, and each src quote is re-read from src/lib/LibMemoryKV.sol at this head.
• Category check: the issue asks that comments state present behaviour and names 7 sites. All 7 are present-tense on this head: has, hashWord, assertNear (deleted by Gas tests pin absolute figures that hold only for this build; README and a src comment state absolute gas #172), testExportLeavesTheStoreIntact, the three getset boundary tests, and the crossFunction header. Per the ruling, the whole-repo sweep covers mutant/fault framing, alternatives not built, history, stale quotes and design rationale. There are 83 sites changed, and every candidate left is listed with its reason below.

Sites changed (83)

• src/lib/LibMemoryKV.sol (4): MemoryKVOverflow drops the truncation counterfactual. MemoryKVLengthOverflow drops the "without this…" counterfactual. The slither cyclomatic comment now states what slither counts. The scoping-blocks comment now states the scoping rule.
• test/lib/LibMemoryKVHandle.t.sol (2): the craft-vs-insert sentinel sentence, and the testCraftNodeRevertsAboveTheWidestHeadPointer hypothetical.
• test/lib/LibMemoryKVSlow.t.sol (1): drops "An allocation short of…".
• array (2 inline): the round-trip value read, and the count-per-pair loop.
• bisect (14): the header (the coupling argument, "two layers", the named-test failure-mode paragraph, "kept anyway, for the name on the failure", "chosen rather than searched for", and the unrewound-pointer counterfactual), VALUE_BASE, checkExportedPairs, checkSoleSlot, testHighSlot14SoleExport, checkSlotPair, testHighSlots1213Siblings, testRootSplitBoundarySlots7And8, testHighSlot14WithBothOrdinaryLeaves, exportMatches (drops the swapped-blocks aside and the "named tests above" comparison), checkEveryOccupancy ("rather than once per combination"), testKeyConstantsLandWhereClaimed, testEveryOccupancyCombinationExportedFromHighPointers, testLengthIsNotRoutedAsAPointer.
• crossFunction (1): the testSaturatedHandleCrossesTheCallBoundaryBitForBit "is a different number" clause. The header is main's (test: crossFunction header states what its tests pin #185).
• errorAbi (1): the header describes what the tests build, not what other tests miss.
• exportAlloc (8): the header, testExportEmptyAllocatesOnlyTheHeader, testExportAllocatesExactlyHeaderPlusTwoWordsPerPair, testExportWritesEveryAllocatedWord, testExportCopiesZeroValue, testExportCopiesZeroKey, testExportAppendsAcrossLists, testExportLeavesTheStoreIntact.
• exportWalk (2): the header drops the src NatSpec it quoted as a reason ("because …") and states what the export reads. The values NatSpec had a false "share no bits" claim and now reads "distinct from both keys and from each other".
• frameLocal (1): getWithFillerExternal ("so a decode that moved fails loudly").
• gasClaims (3): COLLIDING_SLOT drops "Any list would do", testExportGasIsUniformAcrossSlots drops "Delete any of those tests and…", and the hoisted-local inline comment.
• getMatch (3): the header, testGetRequiresEveryBitOfTheKey, testGetMissOverAnOccupiedListReportsNoValue.
• getScratch (1): the header drops "a lookup that built the same hash in either word instead…".
• getWalk (4): the header, testGetAgreesWithSetOnWhichNodeIsTheKey, testGetReadsATailNodeWhenOnlyItMatches, testGetHitLeavesMemoryUntouched.
• getset (1): testGetReadsAValueWordAboveTheBound drops the truncating-get counterfactual.
• handleAlias (1): the header drops "a store that started copying on write…".
• has (2): the header drops "rather than repeating how the walk finds it". testHasAKeyWhoseValueIsZero now states what has and get report.
• saturate (1 inline): the count-per-pair loop.
• setCapacity (5): the header, fillEmptyFrameExternal, fillAfterUnrelatedAllocationExternal, testSetOverflowsOnePairPastAnOtherwiseEmptyFramesCapacity (drops "the form a caller can measure itself against"), testOneUnrelatedWordMakesAnEmptyFramesCapacityOverflow (drops "A pair count is therefore not a capacity" and keeps the fact it rests on).
• setInsert (8): the header, testSetInsertWritesThreeWordsAtTheFreeMemoryPointer, testSetInsertWritesEveryWordOfTheNode, testSetInsertPrependsWithinOneList (NatSpec and inline), testSetInsertWordCountPastAByte, testSetOverflowPayloadIsTheOffendingPointerNotTheBound, testSetInsertIsUnreachableWhenTheReturnIsDropped.
• setSlotMask (3): the header now states the addresses the tests use, not what an allocator fails to exercise. ODD_POINTER drops the walk-off-the-end hypothetical. testInsertLeavesTheNeighbouringSlotsLowBit drops "likeliest to reach past".
• setUpdate (5): the header, testSetHashesIntoTheSameListGetReads, testUpdateFindsTheRightNodeAmongColliders, testWordCountCountsDistinctKeysOnly, testSetDistinguishesKeysDifferingInOneBit.
• storageParity (1): PAIRS_PER_STORE states where ten pairs per store put the nodes, not what an uncapped array does.
• types (1): testTopBitCollidersAreTwoKeys.
• wordCountOverflow (8): the header's short-allocation counterfactual, testSetRevertsRatherThanWrappingTheWordCountToZero, testSetAcceptsTheWidestWordCountThatFits, testSetReportsThePointerWhenBothOverflow, testSetAcceptsAWordCountOfExactlyTheBound (drops the add-instead-of-OR guard counterfactual), testSetReportsTheCountWhenTheAddressIsTheWidestValidOne (drops "what separates an address bound of above from one of at"), testSetOverflowsWhenBothValuesAreTheFirstInvalidOne (drops the raised-bound counterfactual), testWordCountBoundIsUnreachableFromAnEmptyStore.

Candidates left, with reason

Sites this sweep first left to other PRs. Every one of those PRs is merged now, so each site is either resolved on main or handled here:

• src toBytes32Array bisect-saving paragraph, and gasClaims BISECT_SAVING/BISECT_SAVING_MAX_PAIRS plus the testExportGasBeatsLinearWalk NatSpec ([A15] [LOW] toBytes32Array's "~1-1.5k gas every export" claim is wrong and its gas pin is one-sided #118, test: pin the export saving as an ordering across occupancy #181): the constants and that test are gone. The src paragraph and the testExportGasSavingFallsAsListsFill NatSpec state the saving's ordering across occupancy, which that test checks. Kept.
• gasClaims padMemory NatSpec ("Without this…") and the "pessimistic" sentence ([A41] [INFO] gasClaims NatSpec says measuring the linear walk first is 'pessimistic'; padMemory makes order irrelevant #144, test: padMemory makes measurement order immaterial #182): resolved on main. padMemory states what it does, and the "pessimistic" sentence is gone.
• gasClaims SLOTS/SHALLOW_SLOT/COLLIDING_SLOT "15" comments ([A20] [LOW] MemoryKV handle layout is spelled as literals and per-file constants, not one definition #123, test: read the handle layout from LibMemoryKV, not literals #192): SLOTS is gone. SHALLOW_SLOT is kept below. COLLIDING_SLOT is changed here.
• types header and the zero-word tests ([A34] [INFO] types.t.sol empty-store layout tests mask the zero word, so their layout claims can't fail #137, test: pin the count and head pointer tiling on a full word #188): resolved on main. The zero-word tests are gone, and the header states what the file tests.
• storageParity ([A33] [INFO] testMultiGetSetSingle never calls get and only checks exported pairs against storage #136, [A12] [LOW] storageParity testMultiGetSetDouble never checks two live stores against each other #115, [A55] [INFO] storageParity test compares against a no-op wrap/unwrap and names a local after get #158: test: pin set sequences against storage in both directions #179, test: check two live stores each against their own storage #186, test: name testSingleGetSet's read-back local got #184): PAIRS_PER_STORE is changed here. The rest is kept below.
• setCapacity capacity-test NatSpec (setCapacity writes the empty-frame capacity as literals in test names, bodies and the revert address #173, test: name the empty-frame capacity EMPTY_FRAME_PAIRS #178): both sites are changed here.

Kept, because each is a present-tense fact checkable against the tree:

• src: the set NatSpec (the dropped-return warning and the 682 ceiling), the combined-bound comment, the p0001 "would have been if it were not the length" leaf comment (it names the lane the length occupies), the has NatSpec, and "Solidity silently copies it as a value type".
• LibMemoryKVKeys: CANDIDATE_LIMIT sizing, hashWord, and slotOf "restated from the layout … rather than read back out of the store" (the oracle source).
• LibMemoryKVSlow: "MUST stay a plain walk", and the assembly-locals comment.
• SetAtFreePointer: the MUST NOT on walking a crossed handle, because "the walk would read the caller's memory". This is the current behaviour of a walk over that handle.
• crossFunction SATURATED_BASE, ENTRY_FREE_POINTER, buildSaturatedExternal, testSaturatedBuilderRefusesKeysReachingPastItsBase and the decoded-array inline comment (main, test: saturated builder refuses keys that reach past its base #187): where the builder puts its nodes, what it checks on entry and when it reverts. "Where the nodes would overwrite keys the loop has yet to read" is what building from SATURATED_BASE over a longer keys does, as with SetAtFreePointer's walk.
• storageParity: the header and testSingleGetSet ("should behave the same as storage") state the property the tests assert. The KV struct NatSpec (main, test: call the storageParity fuzz struct a key/value pair #183) states what the struct holds, and the StorageStore, setBoth, assertMatchesStorage, testSetSequenceMatchesStorage and testTwoLiveStoresEachMatchTheirOwnStorage NatSpec state what each holds or checks.
• getset: the pointer-boundary NatSpec (already present-tense on main).
• array: the gas-row header, and "We hit gas limits".
• bisect: "the root split is the reason" (a structural fact of the tree), POINTER_HIGH_BIT "must carry", the exportMatches bitset mechanism ("caught by its bit already being set"), and the checkEveryOccupancy pointer range.
• exportAlloc: distinctKeys "counted by the test rather than by the thing under test" and the header's N (the oracle source), the pair-order, write-bounds, one-list and "really allocated, not merely written" NatSpec.
• frameLocal header: "the hazard is a wrong number rather than a revert". This is what a read in another frame returns now.
• handleAlias per-test NatSpec, including the branch cross-talk a caller exploring two paths sees.
• errorAbi constants; wordCountOverflow testSetWordCountOverflowPayloadIsTheOffendingCountNotTheBound NatSpec, header paragraph two, and testSetUpdateAtAFullWordCountIsNotRefused.
• setInsert testSetInsertRecordsTheWholePointer ("not a truncation of it" restates "exactly 0xF000").
• gasClaims SHALLOW_SLOT "instead of descending to it" and BRANCH_LAYOUT_GAS (the current tree and optimizer layout), getsetgas header (Gas tests pin absolute figures that hold only for this build; README and a src comment state absolute gas #172's text, merged).

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 36 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 4185290f-8ba5-4576-9cba-4733e5a8eb2c

📥 Commits

Reviewing files that changed from the base of the PR and between 6aa6aea and aab697b.

📒 Files selected for processing (25)

• src/lib/LibMemoryKV.sol
• test/lib/LibMemoryKVHandle.t.sol
• test/lib/LibMemoryKVSlow.t.sol
• test/src/lib/LibMemoryKV.array.t.sol
• test/src/lib/LibMemoryKV.bisect.t.sol
• test/src/lib/LibMemoryKV.crossFunction.t.sol
• test/src/lib/LibMemoryKV.errorAbi.t.sol
• test/src/lib/LibMemoryKV.exportAlloc.t.sol
• test/src/lib/LibMemoryKV.exportWalk.t.sol
• test/src/lib/LibMemoryKV.frameLocal.t.sol
• test/src/lib/LibMemoryKV.gasClaims.t.sol
• test/src/lib/LibMemoryKV.getMatch.t.sol
• test/src/lib/LibMemoryKV.getScratch.t.sol
• test/src/lib/LibMemoryKV.getWalk.t.sol
• test/src/lib/LibMemoryKV.getset.t.sol
• test/src/lib/LibMemoryKV.handleAlias.t.sol
• test/src/lib/LibMemoryKV.has.t.sol
• test/src/lib/LibMemoryKV.saturate.t.sol
• test/src/lib/LibMemoryKV.setCapacity.t.sol
• test/src/lib/LibMemoryKV.setInsert.t.sol
• test/src/lib/LibMemoryKV.setSlotMask.t.sol
• test/src/lib/LibMemoryKV.setUpdate.t.sol
• test/src/lib/LibMemoryKV.storageParity.t.sol
• test/src/lib/LibMemoryKV.types.t.sol
• test/src/lib/LibMemoryKV.wordCountOverflow.t.sol

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}