October spec review - Githubissues

waldemarhorwat commented 2 weeks ago

Here's my partial review of the spec as of October 2024:

I like the removal of the quantum. This greatly simplified the proposal!
[x] "Only a subset of the values specified in IEEE 754-2019 Decimal128 are modeled here, namely, the set of canonicalized values, which can be intuitively understood as decimal numbers without trailing zeroes" and "the full spectrum of values defined by IEEE 754-2019 Decimal128 are not available here, namely non-canonical Decimal128 values, which can be underdtood as values containing trailing zeroes": This is incorrect. IEEE 754 has the notion of non-canonicalized values, but that's a completely different concept related to the bit encoding (an example is a declet between 1000 and 1023) and not relevant here. What we're doing instead is representing all IEEE 754 values but not providing any operations to distinguish mathematically equal members of a cohort analogously to how we're not providing any operations to distinguish between different IEEE 754 NaN's.
[x] Italicize v.

[ ] As I mentioned before, the Number-to-String-to-Decimal conversion is the correct choice for most uses but we should also provide some way to convert a Number to the closest representable Decimal. It's not at all obvious how to do this using toFixed or toPrecision in a way that works correctly in all cases.

[x] CanonicalizeDecimalString: "Let noTrailingZeroes be the shortest substring of rhs that does not terminate with a sequence of the code point 0x0030 (DIGIT ZERO)": This will always be the empty string. What we want is the longest substring.

Decimal128ToDecimalString has many bugs:

[x] "Let q be the smallest integer such that argument × 10^−q is an integer": There is no such integer — Given any such q, q – 1 will be even smaller and still satisfy that condition. Presumably we want the largest integer instead.
[ ] Integral Decimal values divisible by 10 are handled incorrectly. Decimal128ToDecimalString(1200) will return the string "12".
[ ] The handling of fractions makes no sense: "Let nonIntegerPart be the substring of digits from numDigits" will always produce the empty string given that numDigits is defined as the length of digits.
[ ] Decimal values with magnitude less than 0.1 are handled incorrectly.
[x] Italics formatting errors in the last few lines.

Decimal128ToExponentialString:

[x] "Let q be the smallest integer such that argument × 10^−q is an integer": There is no such integer — Given any such q, q – 1 will be even smaller and still satisfy that condition. Presumably we want the largest integer instead.
[ ] Similar problems as in Decimal128ToDecimalString: Decimal128ToExponentialString(1200) will return the string "1.2e1".
[ ] The decimal point is always output for non-zero inputs, resulting in outputs such as "2.e0". Wouldn't "2e0" be preferable?

[x] Decimal128.prototype.mantissa refers to n when it should be s.
[x] Decimal128.prototype.mantissa should use the same definition of e as Decimal128.prototype.exponent — just copy line 7 of Decimal128.prototype.exponent to line 8 of Decimal128.prototype.mantissa. Having one use the exponent and the other use the truncated exponent introduces an unnecessary and very surprising trap for the unwary.

[x] Decimal128.prototype.remainder: "If d2 is –∞_𝔻 or –∞_𝔻: One of the two constants should be +∞_𝔻, not –∞_𝔻.
[x] Decimal128.prototype.remainder: Line 15 uses 𝔽 instead of 𝔻.

[x] Do we really want equals and notEquals to return undefined instead of false and true respectively?
[ ] Decimal128.prototype.lessThan: The note is incorrect.
[x] Decimal128.prototype.greaterThanOrEqual(–∞_𝔻, –∞_𝔻) returns false. It should return true.

[x] Decimal128.prototype.round: Missing ℝ on line 16. See line 12.
[x] Decimal128.prototype.scale10: All four cases in step 7 are identical. Combine them into one.
[x] Decimal128.prototype.scale10: v should be d on line 11.

[x] Decimal128.prototype.toFixed: Missing _ on line 19b.
[ ] Decimal128.prototype.toFixed makes no sense starting at line 24. nonIntegerDigits will always be "" because s can't contain ".".
[ ] Decimal128.prototype.toPrecision: Both 12a and 12b are off by one. 12a should check for precision = 1. 12b should use precision – 1 copies of a zero after the decimal point.
[ ] Decimal128.prototype.toPrecision makes no sense starting at line 14. There is no q. This section should be rewritten and simplified.

jessealama commented 2 weeks ago

Thank you! This looks great. I'm on it. Stay tuned!

jessealama commented 2 weeks ago

A checked box indicates that the comment has been handled in #180.

tc39 / proposal-decimal