Address code review of #5127 #5263
Conversation
|
👍 |
| @@ -82,7 +82,7 @@ namespace ts { | |||
| return node.end - node.pos; | |||
| } | |||
|
|
|||
| export function arrayIsEqualTo<T>(arr1: T[], arr2: T[], comparer?: (a: T, b: T) => boolean): boolean { | |||
| export function arrayIsEqualTo<T>(arr1: T[], arr2: T[], comparer?: (a: T, b: T) => boolean, sortBeforeComparison = false): boolean { | |||
There was a problem hiding this comment.
Where else is this used?
There was a problem hiding this comment.
Several places:
https://github.com/Microsoft/TypeScript/blob/99d448a1672490a1924adbb377cae2363b28fe33/src/compiler/program.ts#L429
https://github.com/Microsoft/TypeScript/blob/99d448a1672490a1924adbb377cae2363b28fe33/src/compiler/program.ts#L450
https://github.com/Microsoft/TypeScript/blob/331d26f40271c7a21c6fa74ffac633b45f653dda/src/harness/loggedIO.ts#L226
| @@ -360,7 +360,7 @@ namespace ts { | |||
| let newFileNames = ts.map(parsedCommandLine.fileNames, compilerHost.getCanonicalFileName); | |||
| let canonicalRootFileNames = ts.map(rootFileNames, compilerHost.getCanonicalFileName); | |||
|
|
|||
| if (!arrayStructurallyIsEqualTo(newFileNames, canonicalRootFileNames)) { | |||
| if (!arrayIsEqualTo(newFileNames.sort(), canonicalRootFileNames.sort())) { | |||
There was a problem hiding this comment.
Can you add a comment that if the file names are not the same, we just throw away the old program and start fresh?
There was a problem hiding this comment.
can newFileNames or canonicalRootFileNames ever be undefined? previously it was handled in arrayIsEqualTo and now it the same situation will lead to crash.
There was a problem hiding this comment.
Considering @vladima's comment and the fact that structrual comparison would probably be used in other places later, it might be better to change it back to a generic option inside the arrayIsEqualTo. Only difference is I'll add an optional comparer and don't do the sort in-place.
There was a problem hiding this comment.
@zhengbli just do newFileNames && newFileNames.sort(). I don't see why one use case has to conflate the way a core function is written.
There was a problem hiding this comment.
If so there will be multiple places to add the null check and comparison (because if newFilenames and canonicalFileNames are both null it should also compare the two), which seems redundant. Plus the added optional parameters don't really have side effects to arrayIsEqualTo.
|
Other than my last comment, 👍 |
| let newArray2 = sortBeforeComparison ? array2.slice().sort(comparer) : array2; | ||
|
|
||
| for (let i = 0; i < array1.length; ++i) { | ||
| let equals = equaler ? equaler(newArray1[i], newArray2[i]) : newArray1[i] === newArray2[i]; |
There was a problem hiding this comment.
You don't need a function to determine if something is equal, comparer should just return 0.
There was a problem hiding this comment.
if you don't want to sort before comparing, you don't always have a comparer to use though.
There was a problem hiding this comment.
You can (and should) just add an overload.
There was a problem hiding this comment.
I imagine you mean:
export function arrayIsEqualTo<T>(array1: T[], array2: T[], sortBeforeComparison?: boolean, comparer?: (a: T, b: T) => number): Boolean;
export function arrayIsEqualTo<T>(array1: T[], array2: T[], equaler?: (a: T, b: T) => boolean): Boolean;That means if I want to use the sortBeforeCompare option, I must supply a comparer which returns int, even when I don't care about the sorting algorithm. Which also means I cannot use existing functions in our code which already return Boolean. I do not think that is better in terms of compatibility.
There was a problem hiding this comment.
Not at all. I mean
export function arrayIsEqualTo<T>(array1: T[], array2: T[], comparer?: (a: T, b: T) => number)): boolean;
export function arrayIsEqualTo<T>(array1: T[], array2: T[], comparer: (a: T, b: T) => number), sortBeforeComparison: boolean): boolean;
Edit by @DanielRosenwasser: #5127