Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

October spec review #179

Closed
26 tasks done
waldemarhorwat opened this issue Oct 8, 2024 · 3 comments · Fixed by #180
Closed
26 tasks done

October spec review #179

waldemarhorwat opened this issue Oct 8, 2024 · 3 comments · Fixed by #180
Assignees

Comments

@waldemarhorwat
Copy link

waldemarhorwat commented Oct 8, 2024

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

Introduction

  • I like the removal of the quantum. This greatly simplified the proposal!

  • "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.

  • Italicize v.

Conversions from Numbers

  • 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.

Conversions to strings

  • 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:

  • "Let q be the smallest integer such that argument × 10q 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.
  • Italics formatting errors in the last few lines.

Decimal128ToExponentialString:

  • "Let q be the smallest integer such that argument × 10q 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?

Queries

  • Decimal128.prototype.mantissa refers to n when it should be s.
  • 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.

Arithmetic

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

Comparisons

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

Rounding and Scaling

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

String formatters

  • 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 jessealama self-assigned this Oct 8, 2024
@jessealama
Copy link
Collaborator

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

@jessealama jessealama linked a pull request Oct 8, 2024 that will close this issue
@jessealama
Copy link
Collaborator

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

@jessealama
Copy link
Collaborator

All items have been handled except the "Conversions from Numbers" bullet point. We do have an emu-note indicating how we handle conversion from Numbers in that case, with indications about what to do if one wishes for non-default behavior. I'm somewhat inclined to keep it that way for the simple reason that doing so allows us to keep the Decimal128 constructor a single-argument, no-options function, for simplicity's sake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants