Open Brian-McBride opened 2 years ago
I don't know what sum
is used for downstream (yet). But my bet would be that the length of the string is what should be used? That or all strings should be zero (never allowing for automatic casting risking NaN)
Maybe something like a method:
calcSum(value) {
if (typeof value === 'number') return value;
if (value === null) return 0;
return value.length;
}
Used in the three places where this same calculation is being done
if (sumValues) sum += sumShift ? this.calcSum(firstValue) >>> sumShift : this.calcSum(firstValue);
Or whatever the value of a string should be in this case.
By the way, the reason there is a string at all comes from readRawValue()
https://github.com/automerge/automerge/blob/08a456cc2ddc79d8c2f35953db0f1fd8ed418d0f/backend/encoding.js#L889-L903
Where readPrefixedString()
returns a string type.
As a sidenote, readRecord
and readRawValues
both say "do not call from outside the class", but it is called outside the class (inside the RLEEncoder class).
Hi @Brian-McBride, I believe sumValues
should only ever be true on encoders/decoders with a number type (int or uint). It shouldn't ever be adding strings. We could make it throw an exception if you try to pass sumValues && this.type !== 'int' && this.type !== 'uint'
.
Looking through the code, there is this line (repeated a few times)
https://github.com/automerge/automerge/blob/08a456cc2ddc79d8c2f35953db0f1fd8ed418d0f/backend/encoding.js#L695
sum
which is a number (defined above aslet sum = 0
.sumShift
can be undefined. This can be solved earlier in the code withconst { count, sumValues, sumShift = 0 } = options;
firstValue
is returned fromdecoder.readValue()
that has way too many types: string, number, null, undefined, voidA little cleanup in the decoder reduces the return types to
string | number | null
filtering down to the line in question wherefirstValue
can be of a number or string type.anyString >>> anyNumber
has a lot of side effects. It really just depends if the value can be cast to a number. If it can be cast, it will work as expected. If it can't it will return0
. Returning zero isn't probably correct assum
will not be incremented as a number would. Worse though, ifsum += sumShift
is falsey, then we end up with sum adding a string to0
. Now we aren't a number but0some-text-string
.Am I wrong here? Or does the RLEEncoder basically incorrectly generate the
sum
of strings when usingsumValues
?