correct documented result bounds for ToPrecision and ToExponential - #299
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
floitsch
left a comment
There was a problem hiding this comment.
LGTM.
thanks.
I reworded the comment slightly.
| // kMaxExponentialDigits + 10 characters (the sign, the digit before the | ||
| // decimal point, the decimal point, the exponent character, the | ||
| // exponent's sign, and at most 3 exponent digits). | ||
| // exponent's sign, and at most 5 exponent digits; an exponent needs at most | ||
| // 3 digits, but min_exponent_width may pad it to 5). |
There was a problem hiding this comment.
| // kMaxExponentialDigits + 10 characters (the sign, the digit before the | |
| // decimal point, the decimal point, the exponent character, the | |
| // exponent's sign, and at most 3 exponent digits). | |
| // exponent's sign, and at most 5 exponent digits; an exponent needs at most | |
| // 3 digits, but min_exponent_width may pad it to 5). | |
| // kMaxExponentialDigits + 8 characters (the sign, the digit before the | |
| // decimal point, the decimal point, the exponent character, the | |
| // exponent's sign, and at most 3 exponent digits). | |
| // If min_exponent_width is greater than 3, the result also needs space | |
| // for those additional padding digits. Given that min_exponent_width | |
| // is clamped to 5, the result might thus have at most 2 additional characters. |
| // kMaxPrecisionDigits + 9 characters when it is returned in exponential | ||
| // format (the sign, the digit before the decimal point, the decimal point, | ||
| // the exponent character, the exponent's sign, and at most 5 exponent | ||
| // digits; an exponent needs at most 3 digits, but min_exponent_width may | ||
| // pad it to 5), and never more than | ||
| // kMaxPrecisionDigits + max_leading_padding_zeroes_in_precision_mode + 2 | ||
| // characters when it is returned in decimal format (the sign, the decimal | ||
| // point, and the leading zeroes, which include the '0' before the point). |
There was a problem hiding this comment.
| // kMaxPrecisionDigits + 9 characters when it is returned in exponential | |
| // format (the sign, the digit before the decimal point, the decimal point, | |
| // the exponent character, the exponent's sign, and at most 5 exponent | |
| // digits; an exponent needs at most 3 digits, but min_exponent_width may | |
| // pad it to 5), and never more than | |
| // kMaxPrecisionDigits + max_leading_padding_zeroes_in_precision_mode + 2 | |
| // characters when it is returned in decimal format (the sign, the decimal | |
| // point, and the leading zeroes, which include the '0' before the point). | |
| // kMaxPrecisionDigits + 7 characters when it is returned in exponential | |
| // format (the sign, the digit before the decimal point, the decimal point, | |
| // the exponent character, the exponent's sign, and at most 3 exponent | |
| // digits). | |
| // If min_exponent_width is greater than 3, the result also needs space | |
| // for those additional padding digits. Given that min_exponent_width | |
| // is clamped to 5, the result might thus have at most 2 additional characters. | |
| // The result has never more than | |
| // kMaxPrecisionDigits + max_leading_padding_zeroes_in_precision_mode + 2 | |
| // characters when it is returned in decimal format (the sign, the decimal | |
| // point, and the leading zeroes, which include the '0' before the point). |
|
Applied your wording to both blocks (only change: restored the dropped indent on one line of the ToExponential suggestion). The test pins the observed lengths rather than the documented totals, so it's unchanged and still passes. |
|
Thanks! |
Repro: with the stock
EcmaScriptConverter,ToPrecision(-0.000001, 2, &builder)emits the 10-character-0.0000010into a builder sized by the header's own rule (precision + 7, plus one for the\0), and ASan reports a heap-buffer-overflow write on the terminating NUL;ToPrecision(-1.2345e-6, 120)does the same at 128 characters against a documented 127.Cause: the
kMaxPrecisionDigits + 7accounting covers only the exponential form, while a decimal result puts the sign, the decimal point and up tomax_leading_padding_zeroes_in_precision_modeleading zeroes on top ofprecision.ToExponentialandToPrecisionalso both claim at most 3 exponent digits, wheremin_exponent_widthis clamped to 5 and pads the exponent to that width.Fix: state the bounds the conversions actually observe. The emitted digits are right in every one of these cases, so the sizes were the only thing wrong and no conversion changes; the test pins the observed maxima so the numbers cannot drift again.