docs: comments state current behaviour

src/lib/LibMemoryKV.sol

@@ -62,21 +62,16 @@ library LibMemoryKV {
     uint256 internal constant NODE_BYTES = 0x60;
 
     /// Thrown when an insert would allocate its node at a pointer above
-    /// `POINTER_MASK`, the widest head pointer a list slot can hold, so the
-    /// slot would truncate it and the bits above the slot would overwrite the
-    /// neighbouring slots and the word count.
+    /// `POINTER_MASK`, the widest head pointer a list slot can hold.
     ///
     /// Only the head is bounded: the rest of the node MAY extend above
     /// `POINTER_MASK`, as every field is reached by full width arithmetic from
     /// the head. An update allocates nothing, so it never throws this.
     /// @param pointer The offending pointer, not the bound it crossed.
     error MemoryKVOverflow(uint256 pointer);
 
-    /// Thrown when an insert would push the word count past `POINTER_MASK`.
-    /// The count is written by shifting into the top bits of `MemoryKV`, so
-    /// without this the sum silently loses its high bit, and `toBytes32Array`,
-    /// which sizes its allocation from the count, then copies every pair it
-    /// walks past the end of that array.
+    /// Thrown when an insert would push the word count past `POINTER_MASK`,
+    /// the widest word count its slot can hold.
     /// @param length The word count the insert would have produced, not the
     /// stored count before it and not the `POINTER_MASK` bound it crossed.
     error MemoryKVLengthOverflow(uint256 length);
@@ -236,8 +231,8 @@ library LibMemoryKV {
     /// @return array All the keys and values copied pairwise into a `bytes32[]`.
     /// The pair order is unspecified and MUST NOT be relied upon; a caller that
     /// needs a canonical form MUST sort.
-    // Slither is not wrong about the cyclomatic complexity but I don't know
-    // another way to implement the bisect and keep the gas savings.
+    // The cyclomatic complexity slither counts is the bisect's branches, one
+    // per node of the tree over the head pointers.
     //slither-disable-next-line cyclomatic-complexity
     function toBytes32Array(MemoryKV kv) internal pure returns (bytes32[] memory array) {
         uint256 mask16 = type(uint16).max;
@@ -268,9 +263,8 @@ library LibMemoryKV {
 
             // Bisect. The gas this tree saves over a linear loop is documented
             // in the NatSpec above.
-            // The internal scoping blocks are to provide some safety against
-            // typos causing the incorrect symbol to be referenced by enforcing
-            // each symbol is as tightly scoped as it can be.
+            // Each symbol is declared in the smallest block that holds every
+            // use of it, so a use outside that block does not compile.
             let cursor := add(array, 0x20)
             {
                 // Remove the length from kv before iffing, so p0 is zero

test/lib/LibMemoryKVHandle.t.sol

@@ -36,8 +36,8 @@ contract LibMemoryKVHandleTest is Test {
     /// A node crafted where an insert into an empty store would put its node,
     /// under a handle naming it with one pair's count, is exactly what that
     /// insert builds: the same handle, the same three words and the same
-    /// allocation. The memory under each is dirtied first, so a word either
-    /// one leaves unwritten reads as the sentinel.
+    /// allocation. The memory under each is dirtied with the same sentinel
+    /// first.
     function testCraftNodeAndHandleWithBuildWhatAnInsertBuilds(MemoryKVKey key, MemoryKVVal value) external pure {
         bytes32 sentinel = keccak256(abi.encode(key, value));
         Pointer at = LibPointer.allocatedMemoryPointer();
@@ -79,8 +79,7 @@ contract LibMemoryKVHandleTest is Test {
     }
 
     /// A node at `LibMemoryKV.POINTER_MASK` still fits a head slot; one byte
-    /// higher does not and reverts rather than handing back an address a handle
-    /// would truncate.
+    /// higher does not, and crafting a node there reverts.
     function testCraftNodeRevertsAboveTheWidestHeadPointer() external {
         assertEq(
             this.craftNodeAtExternal(LibMemoryKV.POINTER_MASK), LibMemoryKV.POINTER_MASK, "the widest head pointer"

test/lib/LibMemoryKVSlow.t.sol

@@ -12,9 +12,7 @@ import {LibMemoryKVSlow} from "test/lib/LibMemoryKVSlow.sol";
 
 contract LibMemoryKVSlowTest is Test {
     /// The linear export takes the array from the free pointer and leaves the
-    /// free pointer past every word it writes into it. An allocation short of
-    /// what the copy writes leaves exported pairs sitting in memory that the
-    /// next allocation hands out, which nothing reading only the array can see.
+    /// free pointer past every word it writes into it.
     function testSlowLinearExportAllocatesWhatItWrites(bytes32[] memory kvs) external pure {
         vm.assume(kvs.length % 2 == 0);
 

test/src/lib/LibMemoryKV.array.t.sol

@@ -132,10 +132,8 @@ contract LibMemoryKVArrayTest is Test {
             assertEq(slowExists, true);
             assertEq(roundExists, true);
             assertEq(slowVal, roundVal);
-            // A lookup on both sides is the same reader twice: one that answers
-            // with the wrong word of a pair answers with it on both sides and
-            // the comparison holds whatever was exported. The value the model
-            // holds is therefore read out of its array by index as well.
+            // The exported value is also the word the model holds after the
+            // key, read out of its array by index.
             assertEq(roundVal, slowKVs[i + 1]);
         }
     }
@@ -154,11 +152,8 @@ contract LibMemoryKVArrayTest is Test {
 
         assertEq(array.length, arrayLinear.length);
 
-        // Counted over every pair at once, a pair exported twice pays for a
-        // pair not exported at all, so each pair is counted on its own. The
-        // linear walk names the pairs because it visits each of the
-        // `LibMemoryKV.LIST_COUNT` slots exactly once, so a list the bisect
-        // reads twice shows up as a linear pair matched twice.
+        // The linear walk visits each list exactly once, and every pair it
+        // exports is counted on its own in the bisect's export: exactly once.
         for (uint256 i = 0; i < arrayLinear.length; i += 2) {
             assertEq(countPair(array, arrayLinear[i], arrayLinear[i + 1]), 1, "each pair exported exactly once");
         }

test/src/lib/LibMemoryKV.bisect.t.sol

@@ -24,34 +24,23 @@ import {setFreePointer, raiseFreePointerTo} from "test/lib/LibFreeMemory.sol";
 /// slot 8+j for j in 0..6 and lane 7 is structurally zero. The tree then
 /// collapses the node that would have covered that zero lane — slot 14 is
 /// reached by `shr(0x20, p00)` with no `and(mask16, ...)` scrub, where the
-/// mirroring leaf of the low half has one.
-/// That omission is sound only while the root split has already zeroed the
-/// length, a coupling between two lines twelve apart, so the split and both
-/// halves are pinned in one file rather than split across files that cannot see
-/// each other's assumptions.
-///
-/// Two layers, and both are wanted.
+/// mirroring leaf of the low half has one. That leaf reads slot 14 alone
+/// because the root split has already zeroed the length.
 ///
 /// The named tests each occupy one known slot, or one known pair, or one known
-/// boundary. A store that populates many slots at once fails as "some pair is
-/// missing" with no indication of which; a store built from one named slot
-/// fails as that slot's key and value, and the test name says which slot.
+/// boundary, and the test name says which.
 ///
-/// The enumerations then run every combination of which slots are occupied.
-/// That is the tree's whole input: each of its twenty eight guards tests a
-/// window of `kv` against zero, and nothing it does branches on what a key or a
-/// value is. `LibMemoryKV.LIST_COUNT` slots is `OCCUPANCY_COMBINATIONS`
-/// combinations, so the enumerations cover the named cases too, and nothing is
-/// sampled. The named cases are kept anyway, for the name on the failure.
+/// The enumerations run every combination of which slots are occupied. That is
+/// the tree's whole input: each of its twenty eight guards tests a window of
+/// `kv` against zero, and nothing it does branches on what a key or a value is.
+/// `LibMemoryKV.LIST_COUNT` slots is `OCCUPANCY_COMBINATIONS` combinations, so
+/// the enumerations cover the named cases too, and nothing is sampled.
 ///
-/// `slotKeys` is what makes either affordable: one constant key per slot, so an
-/// occupancy is chosen rather than searched for, and no test in this file takes
-/// a fuzz input.
+/// `slotKeys` is one constant key per slot, so every test here chooses its
+/// occupancy and none takes a fuzz input.
 ///
 /// Every node of every case an enumeration builds is allocated from a free
-/// memory pointer that is rewound between cases. Unrewound, the
-/// `OCCUPANCY_COMBINATIONS` cases walk that pointer past the
-/// `LibMemoryKV.POINTER_MASK` ceiling `set` enforces after 115 of them.
+/// memory pointer that is rewound between cases.
 contract LibMemoryKVBisectTest is Test {
     using LibMemoryKV for MemoryKV;
 
@@ -76,10 +65,9 @@ contract LibMemoryKVBisectTest is Test {
     /// collide with.
     uint256 internal constant NO_MASK = type(uint256).max;
 
-    /// Values sit above every key, so a copy that reads the key where the value
-    /// belongs, or that reads another slot's value, is a different word. A
-    /// value is this base plus the slot it belongs to, which is how
-    /// `exportMatches` reads a pair's slot back out of it.
+    /// Values sit above every key, so no value word equals a key word. A value
+    /// is this base plus the slot it belongs to, so no two slots share a value,
+    /// which is how `exportMatches` reads a pair's slot back out of it.
     uint256 internal constant VALUE_BASE = 0x100;
 
     /// One key per slot: the smallest positive integer whose 32 byte big endian
@@ -175,10 +163,8 @@ contract LibMemoryKVBisectTest is Test {
     }
 
     /// Exports the store and holds it to the pairs `mask` names, one pair per
-    /// named slot and each exported exactly once. A window that is too wide
-    /// duplicates a pair, one that is too narrow drops it, and a leaf that does
-    /// not carry the cursor forward is overwritten by the next one — all three
-    /// are a count other than one. Order is not checked, per `exportMatches`.
+    /// named slot and each exported exactly once. Order is not checked, per
+    /// `exportMatches`.
     function checkExportedPairs(MemoryKV kv, uint256 mask, bytes32[] memory keys) internal pure {
         bytes32[] memory array = kv.toBytes32Array();
         assertEq(array.length, occupiedCount(mask) * 2, "one pair per occupied slot");
@@ -216,10 +202,8 @@ contract LibMemoryKVBisectTest is Test {
         checkExportedPairs(kv, mask, keys);
     }
 
-    /// A single key in `slot` and nothing else. The exported array must be
-    /// exactly that one pair, so a bisect that never visits `slot`, or that
-    /// reaches it with a mangled pointer, exports something other than the key
-    /// where the key belongs.
+    /// A single key in `slot` and nothing else. The exported array is exactly
+    /// that one pair: the slot's key, then its value.
     function checkSoleSlot(uint256 slot) internal pure {
         bytes32[] memory keys = slotKeys();
         MemoryKV kv = storeForOccupancy(2 ** slot, keys);
@@ -288,17 +272,13 @@ contract LibMemoryKVBisectTest is Test {
     }
 
     /// Slot 14 is the leaf the collapsed path reaches. Alone in the store its
-    /// `LibMemoryKV.SLOT_BITS` bits are the only nonzero bits of `p00`, so a
-    /// shift of the wrong distance yields a pointer into unwritten memory
-    /// rather than the node.
+    /// `LibMemoryKV.SLOT_BITS` bits are the only nonzero bits of `p00`, and the
+    /// export reaches its node through them.
     function testHighSlot14SoleExport() public pure {
         checkSoleSlot(14);
     }
 
-    /// Two known slots and nothing else. A mask or shift that folds one onto
-    /// the other, or a window that covers one twice, changes how often a pair
-    /// appears rather than merely dropping it — which neither slot alone would
-    /// reveal.
+    /// Two known slots and nothing else, each exported exactly once.
     function checkSlotPair(uint256 slotA, uint256 slotB) internal pure {
         checkNamedOccupancy(2 ** slotA | 2 ** slotB);
     }
@@ -330,26 +310,23 @@ contract LibMemoryKVBisectTest is Test {
     }
 
     /// Slots 12 and 13 are the ordinary leaves under the same node as slot 14,
-    /// occupied while slot 14 is not. The collapsed path must read as empty
-    /// here: anything left in the bits above slot 14 turns into a pointer and
-    /// costs the store a pair.
+    /// occupied while slot 14 is not. The collapsed path reads as empty here,
+    /// and the export is exactly the two pairs.
     function testHighSlots1213Siblings() public pure {
         checkSlotPair(12, 13);
     }
 
     /// The two halves of the root split meet between slot 7 and slot 8. Slots 7
     /// and 8 are not siblings — they are the outermost leaves of different
-    /// halves — so one key each side of that boundary is the discriminator for
-    /// the split itself: a window that overlaps exports one of them twice, and
-    /// a window that leaves a gap drops one.
+    /// halves — so this is one key each side of the split, each exported
+    /// exactly once.
     function testRootSplitBoundarySlots7And8() public pure {
         checkSlotPair(7, 8);
     }
 
     /// All three leaves below the node that holds slot 14, so the collapsed
-    /// path and the ordinary path run against each other. A mask that lets the
-    /// collapsed leaf through to the ordinary subtree, or the reverse, changes
-    /// how often a pair appears rather than merely dropping it.
+    /// path and the ordinary path run in one export, and each of the three
+    /// pairs is exported exactly once.
     function testHighSlot14WithBothOrdinaryLeaves() public pure {
         checkNamedOccupancy(2 ** 12 | 2 ** 13 | 2 ** 14);
     }
@@ -382,19 +359,14 @@ contract LibMemoryKVBisectTest is Test {
 
     /// Whether `array` holds exactly the pairs `mask` must export, as a
     /// multiset. `toBytes32Array` documents its pair order as unspecified, so
-    /// position is deliberately not checked: two blocks of the tree swapped
-    /// emit the same pairs in a different order and are not a defect.
+    /// position is not checked.
     ///
     /// Each value is `VALUE_BASE + slot`, so the slot a pair claims is read out
     /// of its own value, and the pair is then held to the key that slot must
     /// carry. Collecting those slots into a bitset and comparing it to `mask` is
     /// multiset equality without a scan per pair: a slot exported twice is
     /// caught by its bit already being set, and one dropped, duplicated or
     /// invented by the bitsets differing.
-    ///
-    /// The named tests above check the same thing through `countPair`, which
-    /// costs a scan per pair but names the slot it checked. Here the whole
-    /// point is `OCCUPANCY_COMBINATIONS` cases, so the check is the cheap one.
     function exportMatches(bytes32[] memory array, uint256 mask, bytes32[] memory keys) internal pure returns (bool) {
         if (array.length % 2 != 0) {
             return false;
@@ -422,8 +394,8 @@ contract LibMemoryKVBisectTest is Test {
     /// Exports all `OCCUPANCY_COMBINATIONS` occupancy combinations and checks
     /// each against the pairs that combination must produce. The check is plain
     /// arithmetic and the first mask that fails it is carried out of the loop,
-    /// so the assertion machinery runs once rather than once per combination and
-    /// the mask that failed is what the assertion reports.
+    /// so the assertion machinery runs once and the mask that failed is what the
+    /// assertion reports.
     ///
     /// Every case is built from one free memory pointer, `free`, taken after
     /// `slotKeys` has allocated, so every node of every case lies in
@@ -454,16 +426,13 @@ contract LibMemoryKVBisectTest is Test {
         assertEq(failed, NO_MASK, "occupancy mask whose export did not match");
     }
 
-    /// The slot key constants are the whole reason a combination can be
-    /// chosen rather than searched for, and every test below reads its result
-    /// through the slot each key claims. Should the hash that `get` and `set`
-    /// share ever move, the keys stop naming the slots they claim and the tests
-    /// silently cover something other than what they report.
+    /// Each key constant hashes into the slot it is claimed for, and every
+    /// chain key into slot 0. Every test in this file reads its result through
+    /// the slot each key claims.
     ///
-    /// Occupying all of them at once is what makes them
-    /// `LibMemoryKV.LIST_COUNT` distinct slots rather than that many keys that
-    /// each hash somewhere. `set` routes on the key alone, so that plus one key
-    /// per slot is every occupancy.
+    /// Occupying all of them at once populates `LibMemoryKV.LIST_COUNT`
+    /// distinct slots. `set` routes on the key alone, so that plus one key per
+    /// slot is every occupancy.
     function testKeyConstantsLandWhereClaimed() public pure {
         bytes32[] memory keys = slotKeys();
         for (uint256 slot = 0; slot < LibMemoryKV.LIST_COUNT; slot++) {
@@ -483,18 +452,18 @@ contract LibMemoryKVBisectTest is Test {
     }
 
     /// Every occupancy combination again with every pointer at or above
-    /// `POINTER_HIGH_BIT`, so both states of a pointer's top bit are enumerated
-    /// rather than left to wherever the allocator happened to be.
+    /// `POINTER_HIGH_BIT`, so both states of a pointer's top bit are
+    /// enumerated.
     function testEveryOccupancyCombinationExportedFromHighPointers() public pure {
         raiseFreePointerTo(POINTER_HIGH_BIT);
         checkEveryOccupancy(POINTER_HIGH_BIT, LibMemoryKV.POINTER_MASK);
     }
 
     /// The length is not a slot. It sits directly above slot 14, which is the
-    /// leaf the collapsed path reaches without scrubbing, so a root split that
-    /// carried the length into the tree would read it as that leaf's pointer.
-    /// Every pair here is on one low list, which leaves the whole high half
-    /// empty while the length is far larger than any single insert makes it.
+    /// leaf the collapsed path reaches without scrubbing, and the export reads
+    /// no pointer out of it. Every pair here is on one low list, which leaves
+    /// the whole high half empty while the length is far larger than any single
+    /// insert makes it.
     ///
     /// It is also the only list built longer than one node, so it is the only
     /// place the walk down a list is exercised at all. Which order it comes back

test/src/lib/LibMemoryKV.crossFunction.t.sol

@@ -66,11 +66,9 @@ contract LibMemoryKVCrossFunctionTest is Test {
     }
 
     /// A handle with every head pointer slot and the word count in use arrives
-    /// on the other side of an external call bit for bit. The word it must equal is
-    /// computed here from the addresses the building frame was forced to
-    /// allocate at, so a handle that lost the word count, or a slot, or the top
-    /// bit of one pointer, is a different number rather than a store that
-    /// merely reads oddly.
+    /// on the other side of an external call bit for bit. It equals the word
+    /// computed here from the word count and the addresses the building frame
+    /// allocates its nodes at.
     function testSaturatedHandleCrossesTheCallBoundaryBitForBit(bytes32 seed) external view {
         bytes32[] memory keys = new bytes32[](LibMemoryKV.LIST_COUNT);
         uint256 expected = (LibMemoryKV.LIST_COUNT * 2) << LibMemoryKV.COUNT_BIT_OFFSET;