tree: Refactor RangeMap to support generic key types #23560
Conversation
github-actions
bot
added
area: dds
| this.tree.clear(); | ||
| } | ||
|
|
||
| public get(start: K, length: number): RangeQueryEntry<K, V>[] { |
There was a problem hiding this comment.
Would be nice to have doc comments on this to clarify the contract. Especially with respect to the first and last RangeQueryEntry returned
| result.push({ start, length: overlappingLength, value }); | ||
| nextKey = this.offsetKey(lastEntryKey, 1); |
There was a problem hiding this comment.
It may not be strictly necessary for correctness of this code (I have a hard time telling), but it seems like it would be good to update remainingLength here at least to avoid confusion.
| } | ||
|
|
||
| const [key, { length: entryLength, value }] = entry; | ||
| const lastEntryKey = this.offsetKey(key, entryLength); |
There was a problem hiding this comment.
Nit: move the computation of lastEntryKey below the if block since it may not be used if we do break out of the loop
| @@ -36,16 +99,16 @@ export class RangeMap<T> { | |||
| * @returns A RangeQueryResult containing the value associated with `start`, | |||
| * and the number of consecutive keys with that same value. | |||
There was a problem hiding this comment.
| * and the number of consecutive keys with that same value. | |
| * and the number of consecutive keys with that same value (at least 1, at most `length`). |
| const cmpRevision = compareRevisions(a.revision, b.revision); | ||
| if (cmpRevision !== 0) { | ||
| return cmpRevision * Number.POSITIVE_INFINITY; | ||
| } | ||
|
|
||
| return a.localId - b.localId; |
There was a problem hiding this comment.
Once you've dealt with the target part, can you reuse subtractChangeAtomIds from src\core\rebase\types.ts?
| /** | ||
| * Returns a new map which contains the entries from both input maps. | ||
| */ | ||
| public static mergeMaps<K, V>(a: RangeMap<K, V>, b: RangeMap<K, V>): RangeMap<K, V> { |
There was a problem hiding this comment.
Action verbs like "merge" suggest that side effects may occur. (This would be less ambiguous if we had readonly versions of the RangeMap types.)
Either way, how about naming this union instead?
| getCrossFieldTargetFromMove(markEffect), | ||
| markEffect.revision, | ||
| markEffect.id, | ||
| count, | ||
| true, |
There was a problem hiding this comment.
This change seems unrelated to the PR. Was there a bug here? If so, some test coverage for it would be good. Maybe this change is best kept for another PR.
Description
This PR refactors RangeMap to support arbitrary types of keys. Various uses of RangeMap nested inside of a map of revisions are flattened into a single range map keyed on ChangeAtomId. ModularChangeset.crossFieldKeys is also refactored to use a RangeMap, and some functionality has been added to RangeMap to support this use case.