microsoft / tiny-calc

Tiny expression evaluator
MIT License
39 stars 9 forks source link

API: IVectorShapeWriter.splice() is inconsistent with Array.splice() #84

Closed DLehenbauer closed 3 years ago

DLehenbauer commented 3 years ago

Array.splice() accepts a set of items to insert, where IVectorShapeWriter takes a number of default items / holes to insert.

Array.splice(start: number, deleteCount: number, ...items: T[]): T[];
IVectorShapeWriter.splice(start: number, deleteCount: number, insertCount: number): void;

The motivations for this inconsistency were:

  1. The spread operator ('...items[]') can lead to stack exhaustion when the number of items inserted is large on v8 (v12 x64).
  2. Providing the items up front encourages potentially unnecessary array allocations/copies.
  3. Philosophically, IVectorShapeWriter should be agnostic to the type of items stored (i.e., should not have a generic T param)

(Issues 1 & 2 have proven to be real concerns for IMatrix, which has similar spliceRows/Cols APIs)

However, in practice this API difference has proven to be error-prone for IVector. This is because when T=number, both signatures will accept '.splice(0, 0, 3)', which Array would interpret as insert '3' and IVector interprets as insert 3 holes.

(It's also just clunky/annoying to have to call splice() and then use setItem() to fill in the holes.)

My current thinking is that we should rename IShapeWriter.splice to avoid confusion (and perhaps reintroduce a compatible splice on I*Writer.)

...if only I could think of a good name... The best I have so far is 'resize()', but that might be a discovery issue as 'resize()' is usually used for modifying length only.

PS - There is a second inconsistency where IVectorShapeWriter.splice() returns void instead of an array of the removed items, but I believe here there is far more upside.