Skip to content

Commit

Permalink
fix: redundantly assert no duplicates within set merge
Browse files Browse the repository at this point in the history
  • Loading branch information
erights committed Jan 24, 2022
1 parent e12f321 commit 55a4bd8
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 6 deletions.
32 changes: 28 additions & 4 deletions packages/store/src/keys/copySet.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,27 @@ const { details: X } = assert;
/**
* @template T
* @param {T[]} elements
* @param {FullCompare=} fullCompare If provided and `elements` is already known
* to be sorted by this `fullCompare`, then we should get a memo hit rather
* than a resorting. However, currently, we still enumerate the entire array
* each time.
*
* TODO: If doing this reduntantly turns out to be expensive, we
* could memoize this no-duplicate finding as well, independent
* of the `fullOrder` use to reach this finding.
* @param {Checker=} check
* @returns {boolean}
*/
const checkNoDuplicates = (elements, check = x => x) => {
const checkNoDuplicates = (
elements,
fullCompare = undefined,
check = x => x,
) => {
// This fullOrder contains history dependent state. It is specific
// to this one `merge` call and does not survive it.
const fullCompare = makeFullOrderComparatorKit().antiComparator;
// to this one call and does not survive it.
// TODO Once all our tooling is ready for `&&=`, the following
// line should be rewritten using it.
fullCompare = fullCompare || makeFullOrderComparatorKit().antiComparator;

elements = sortByRank(elements, fullCompare);
const { length } = elements;
Expand All @@ -35,6 +49,16 @@ const checkNoDuplicates = (elements, check = x => x) => {
return true;
};

/**
* @template T
* @param {T[]} elements
* @param {FullCompare=} fullCompare
* @returns {void}
*/
export const assertNoDuplicates = (elements, fullCompare = undefined) => {
checkNoDuplicates(elements, fullCompare, assertChecker);
};

/**
* @param {Passable[]} elements
* @param {Checker=} check
Expand All @@ -53,7 +77,7 @@ export const checkElements = (elements, check = x => x) => {
X`The keys of a copySet or copyMap must be sorted in reverse rank order: ${elements}`,
);
}
return checkNoDuplicates(elements, check);
return checkNoDuplicates(elements, undefined, check);
};
harden(checkElements);

Expand Down
8 changes: 6 additions & 2 deletions packages/store/src/keys/merge-set-operators.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
makeFullOrderComparatorKit,
sortByRank,
} from '../patterns/rankOrder.js';
import { makeSetOfElements } from './copySet.js';
import { assertNoDuplicates, makeSetOfElements } from './copySet.js';

const { details: X } = assert;

Expand All @@ -23,7 +23,7 @@ const { details: X } = assert;
*
* @template T
* @param {T[]} elements
* @param {FullCompare} rankCompare
* @param {RankCompare} rankCompare
* @param {FullCompare} fullCompare
* @returns {Iterable<T>}
*/
Expand Down Expand Up @@ -58,6 +58,10 @@ const windowResort = (elements, rankCompare, fullCompare) => {
const similarRun = elements.slice(i, j);
i = j;
const resorted = sortByRank(similarRun, fullCompare);
// Providing the same `fullCompare` should cause a memo hit
// within `assertNoDuplicates` enabling it to avoid a
// redundant resorting.
assertNoDuplicates(resorted, fullCompare);
optInnerIterator = resorted[Symbol.iterator]();
return optInnerIterator.next();
} else {
Expand Down

0 comments on commit 55a4bd8

Please sign in to comment.