Skip to content

Commit

Permalink
fix: review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@erights
erights committed Jan 29, 2022
1 parent 7aec55c commit ce019dc
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 21 deletions.
39 changes: 29 additions & 10 deletions packages/store/src/keys/merge-bag-operators.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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],
Expand All @@ -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],
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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;
}
};
Expand Down
41 changes: 30 additions & 11 deletions packages/store/src/keys/merge-set-operators.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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;
}
};
Expand All @@ -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);
Expand All @@ -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);
}
}
Expand Down

0 comments on commit ce019dc

Please sign in to comment.