From ce019dc6838c2205670aa70c08d06d8e7195bf5f Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Fri, 28 Jan 2022 20:30:56 -0800 Subject: [PATCH] fix: review comments --- .../store/src/keys/merge-bag-operators.js | 39 +++++++++++++----- .../store/src/keys/merge-set-operators.js | 41 ++++++++++++++----- 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/packages/store/src/keys/merge-bag-operators.js b/packages/store/src/keys/merge-bag-operators.js index 9d97d5d7a0c2..70fb99cefff8 100644 --- a/packages/store/src/keys/merge-bag-operators.js +++ b/packages/store/src/keys/merge-bag-operators.js @@ -8,7 +8,7 @@ import { } from '../patterns/rankOrder.js'; import { assertNoDuplicateKeys, makeBagOfEntries } from './copyBag.js'; -const { details: X } = assert; +const { details: X, quote: q } = assert; // Based on merge-set-operators.js, but altered for the bag representation. // TODO share more code with merge-set-operators.js, rather than @@ -72,6 +72,11 @@ const bagWindowResort = (bagEntries, rankCompare, fullCompare) => { // within `assertNoDuplicates` enabling it to avoid a // redundant resorting. assertNoDuplicateKeys(resorted, fullCompare); + // This is the raw JS array iterator whose `.next()` method + // does not harden the IteratorResult, in violation of our + // conventions. Fixing this is expensive and I'm confident the + // unfrozen value does not escape this file, so I'm leaving this + // as is. optInnerIterator = resorted[Symbol.iterator](); return optInnerIterator.next(); } else { @@ -83,6 +88,13 @@ const bagWindowResort = (bagEntries, rankCompare, fullCompare) => { }; /** + * Returns an iterable whose iteration results are [key, xCount, yCount] tuples + * representing the next key in the local full order, as well as how many + * times it ocurred in the x input iterator and the y input interator. + * + * For sets, these counts are always 0 or 1, but this representation + * generalizes nicely for bags. + * * @template T * @param {[T,bigint][]} xbagEntries * @param {[T,bigint][]} ybagEntries @@ -97,13 +109,21 @@ const merge = (xbagEntries, ybagEntries) => { const ys = bagWindowResort(ybagEntries, compareAntiRank, fullCompare); return harden({ [Symbol.iterator]: () => { - const xi = xs[Symbol.iterator](); + // These six `let` variables are buffering one ahead from the underlying + // iterators. Each iteration reports one or the other or both, and + // then refills the buffers of those it advanced. /** @type {T} */ let x; let xc; let xDone; + /** @type {T} */ + let y; + let yc; + let yDone; + + const xi = xs[Symbol.iterator](); const nextX = () => { - assert(!xDone); + assert(!xDone, X`Internal: nextX should not be called once done`); ({ done: xDone, value: [x, xc], @@ -112,12 +132,8 @@ const merge = (xbagEntries, ybagEntries) => { nextX(); const yi = ys[Symbol.iterator](); - /** @type {T} */ - let y; - let yc; - let yDone; const nextY = () => { - assert(!yDone); + assert(!yDone, X`Internal: nextY should not be called once done`); ({ done: yDone, value: [y, yc], @@ -156,7 +172,7 @@ const merge = (xbagEntries, ybagEntries) => { nextX(); } else { // y is earlier, so report it - assert(comp > 0); + assert(comp > 0, X`Internal: Unexpected comp ${q(comp)}`); value = [y, 0n, yc]; nextY(); } @@ -216,7 +232,10 @@ const bagIterCompare = xyi => { } else if (loneY) { return -1; } else { - assert(!loneX && !loneY); + assert( + !loneX && !loneY, + X`Internal: Unexpected lone pair ${q([loneX, loneY])}`, + ); return 0; } }; diff --git a/packages/store/src/keys/merge-set-operators.js b/packages/store/src/keys/merge-set-operators.js index de48cef22b89..6ba5ed63b085 100644 --- a/packages/store/src/keys/merge-set-operators.js +++ b/packages/store/src/keys/merge-set-operators.js @@ -8,7 +8,7 @@ import { } from '../patterns/rankOrder.js'; import { assertNoDuplicates, makeSetOfElements } from './copySet.js'; -const { details: X } = assert; +const { details: X, quote: q } = assert; /** * Asserts that `elements` is already rank sorted by `rankCompare`, where there @@ -62,6 +62,11 @@ const windowResort = (elements, rankCompare, fullCompare) => { // within `assertNoDuplicates` enabling it to avoid a // redundant resorting. assertNoDuplicates(resorted, fullCompare); + // This is the raw JS array iterator whose `.next()` method + // does not harden the IteratorResult, in violation of our + // conventions. Fixing this is expensive and I'm confident the + // unfrozen value does not escape this file, so I'm leaving this + // as is. optInnerIterator = resorted[Symbol.iterator](); return optInnerIterator.next(); } else { @@ -73,6 +78,13 @@ const windowResort = (elements, rankCompare, fullCompare) => { }; /** + * Returns an iterable whose iteration results are [key, xCount, yCount] tuples + * representing the next key in the local full order, as well as how many + * times it ocurred in the x input iterator and the y input interator. + * + * For sets, these counts are always 0 or 1, but this representation + * generalizes nicely for bags. + * * @template T * @param {T[]} xelements * @param {T[]} yelements @@ -87,22 +99,26 @@ const merge = (xelements, yelements) => { const ys = windowResort(yelements, compareAntiRank, fullCompare); return harden({ [Symbol.iterator]: () => { - const xi = xs[Symbol.iterator](); + // These four `let` variables are buffering one ahead from the underlying + // iterators. Each iteration reports one or the other or both, and + // then refills the buffers of those it advanced. /** @type {T} */ let x; let xDone; + /** @type {T} */ + let y; + let yDone; + + const xi = xs[Symbol.iterator](); const nextX = () => { - assert(!xDone); + assert(!xDone, X`Internal: nextX should not be called once done`); ({ done: xDone, value: x } = xi.next()); }; nextX(); const yi = ys[Symbol.iterator](); - /** @type {T} */ - let y; - let yDone; const nextY = () => { - assert(!yDone); + assert(!yDone, X`Internal: nextY should not be called once done`); ({ done: yDone, value: y } = yi.next()); }; nextY(); @@ -138,7 +154,7 @@ const merge = (xelements, yelements) => { nextX(); } else { // y is earlier, so report it - assert(comp > 0); + assert(comp > 0, X`Internal: Unexpected comp ${q(comp)}`); value = [y, 0n, 1n]; nextY(); } @@ -192,7 +208,10 @@ const iterCompare = xyi => { } else if (loneY) { return -1; } else { - assert(!loneX && !loneY); + assert( + !loneX && !loneY, + X`Internal: Unexpected lone pair ${q([loneX, loneY])}`, + ); return 0; } }; @@ -203,7 +222,7 @@ const iterUnion = xyi => { if (xc >= 0n) { result.push(m); } else { - assert(yc >= 0n); + assert(yc >= 0n, X`Internal: Unexpected count ${q(yc)}`); // if x and y were both ready, then they were equivalent and // above clause already took care of it. Otherwise push here. result.push(m); @@ -219,7 +238,7 @@ const iterDisjointUnion = xyi => { if (xc >= 1n) { result.push(m); } else { - assert(yc >= 1n); + assert(yc >= 1n, X`Internal: Unexpected count ${q(yc)}`); result.push(m); } }