Bug 87360

Created attachment 143778 [details] Patch 1 - Check makefile and UINT64_C macro

Comment on attachment 143778 [details] Patch 1 - Check makefile and UINT64_C macro Attachment 143778 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12802112

Created attachment 143942 [details] Patch 2

Comment on attachment 143942 [details] Patch 2 * Build on Qt is succeeded. * Others are seemed to be OK, because I've just added include directive to Decimal.cpp. ** It seems Qt doesn't include ICU files. Could you review this patch? Thanks in advance.

Comment on attachment 143942 [details] Patch 2 All build bot is passed. Could you review this patch? Thanks in advance.

Comment on attachment 143942 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=143942&action=review > Source/WTF/wtf/MathExtras.h:313 > > +#ifndef UINT64_C > +#if COMPILER(MSVC) > +#define UINT64_C(c) c ## ui64 > +#else > +#define UINT64_C(c) c ## ull > +#endif > +#endif > + > Please make another patch for this change with WebCore/Modules/websockes/WebSocketFrame.cpp fix. > Source/WebCore/platform/Decimal.cpp:46 > +using namespace std; We recently changed the style rule. We should not use "using namespace" for std. http://www.webkit.org/coding/coding-style.html#using-in-cpp > Source/WebCore/platform/Decimal.cpp:48 > +static const uint64_t MaxCoefficient = UINT64_C(0x16345785D89FFFF); // 999999999999999999; // 18 9's Two '//' in one line. > Source/WebCore/platform/Decimal.cpp:72 > + LhsIsinfinity, > + RhsIsinfinity, should be LHSIsInfinity, RHSIsInfinity, http://www.webkit.org/coding/coding-style.html#names-basic > Source/WebCore/platform/Decimal.cpp:82 > + ResultIsLhs, > + ResultIsRhs, Should be ResultIsLHS, ResultIsRHS, http://www.webkit.org/coding/coding-style.html#names-basic > Source/WebCore/platform/Decimal.cpp:88 > + const Decimal& m_lhs; > + Result m_result; > + const Decimal& m_rhs; Should be reordered. const Decimal& m_lhs; const Decimal& m_rhs; Result m_result; to minimize objects. > Source/WebCore/platform/Decimal.cpp:94 > + UInt128(uint64_t, uint64_t); should have argument names. > Source/WebCore/platform/Decimal.cpp:101 > + static UInt128 multiply(uint64_t, uint64_t); ditto. > Source/WebCore/platform/Decimal.cpp:790 > + #define HandleChar(ch1, next, ...) \ I prefer HandleCharAndBreak(). > Source/WebCore/platform/Decimal.cpp:799 > + #define ExpectChar(ch1, next, ...) \ > + HandleChar(ch1, next, __VA_ARGS__); \ > + return nan(); Hiding non-conditional 'return' in a macro is not good for readability. Please remove ExpectChar. > Source/WebCore/platform/Decimal.cpp:1109 > +} // WebCore should be "namespace WebCore" > Source/WebCore/platform/Decimal.h:39 > +// This class represents decimal base floating pointer number with 32 bit > +// unsigend integer and 64 bit unsigned integer. pointer -> point? The phrase "with ..." is an implementation detail. I think we had better make another sentence for it. > Source/WebCore/platform/Decimal.h:48 > +// 76543210 76543210 > +// S0000000 0000MEEE > +// S = sign (1 bit), 0 = positive, 1 = negative > +// E = exponent (11 bit) The figure looks like 16 bit. What's 'M' in this figure? Why three 'E's represent 11 bit in this figure? Why do we need pack them? Why not bool&in16_t? > Source/WebCore/platform/Decimal.h:56 > + static int const Emax = 2047; // largest exponent value > + static int const Ebias = 1023; // subtracted from encoded exponent should be ExponentMax or MaxExponent. should be ExponentBias. They can be moved to Decimal.cpp, right? > Source/WebCore/platform/Decimal.h:57 > + static int const Precision = 18; It can be moved to Decimal.cpp. > Source/WebCore/platform/Decimal.h:64 > + enum FormatClass { > + ClassInfinity, > + ClassNormal, > + ClassNaN, > + ClassZero, > + }; Should it be public? > Source/WebCore/platform/Decimal.h:68 > + enum Sign { > + Plus, > + Minus, I prefer Positive/Negative. > Source/WebCore/platform/Decimal.h:71 > + struct EncodedData { Should it be public? > Source/WebCore/platform/Decimal.h:73 > + explicit EncodedData(uint32_t); > + EncodedData(uint64_t, int, Sign); We should add argument names to uint32_t, uint64_t, and int. Their roles are unclear. > Source/WebCore/platform/Decimal.h:89 > + Decimal(); What value does an object constructed by this? > Source/WebCore/platform/Decimal.h:91 > + Decimal(uint64_t, int, Sign); What is 'int'? Need an argument name or a comment. > Source/WebCore/platform/Decimal.h:113 > + Decimal& operator =(const Decimal&); > + Decimal& operator +=(const Decimal&); > + Decimal& operator -=(const Decimal&); > + Decimal& operator *=(const Decimal&); > + Decimal& operator /=(const Decimal&); > + > + Decimal operator -() const; > + > + bool operator ==(const Decimal&) const; > + bool operator !=(const Decimal&) const; > + bool operator <(const Decimal&) const; > + bool operator <=(const Decimal&) const; > + bool operator >(const Decimal&) const; > + bool operator >=(const Decimal&) const; > + > + Decimal operator +(const Decimal&) const; > + Decimal operator -(const Decimal&) const; > + Decimal operator *(const Decimal&) const; > + Decimal operator /(const Decimal&) const; We usually don't put a space after 'operator' > Source/WebCore/platform/Decimal.h:115 > + int exponent() const; What's returned by this? Is it biased? > Source/WebCore/platform/Decimal.h:116 > + FormatClass formatClass() const; Should it be public? > Source/WebCore/platform/Decimal.h:122 > + const EncodedData& value() const { return m_data; } Should it be public? > Source/WebCore/platform/Decimal.h:152 > +} // WebCore should be "namespace WebCore" > Source/WebKit/chromium/tests/DecimalTest.cpp:52 > +namespace DecimalTest { I don't think we need to enclose everything with a dedicated namespace.

Created attachment 144276 [details] Patch 3

Comment on attachment 144276 [details] Patch 3 I updated the patch to follow reviewer's comments. Could you review again? Thanks in advance.

Comment on attachment 144276 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=144276&action=review > Source/WebCore/platform/Decimal.cpp:58 > + Bothinfinity, should be Both*I*nfinity > Source/WebCore/platform/Decimal.cpp:61 > + LHSIsinfinity, > + RHSIsinfinity, should be LHSIsInfinity / RHSIsInfinity. > Source/WebCore/platform/Decimal.cpp:103 > + while (x >= powerOf10) { nit: for (uint64_t poweOfTen = 1; x >= powerOfTen; powerOfTen += 10) { > Source/WebCore/platform/Decimal.cpp:105 > + if (powerOf10 >= uint64_t(-1) / 10) uint64_t(-1) should be std::numeric_limits<uint64_t>::max() for code readability. > Source/WebCore/platform/Decimal.cpp:112 > +static uint64_t multiplyHigh(uint64_t u, uint64_t v) Please make this a member of UInt128 class. > Source/WebCore/platform/Decimal.cpp:120 > + uint64_t u0 = u & mask; > + uint64_t u1 = u >> n2; > + uint64_t v0 = v & mask; > + uint64_t v1 = v >> n2; u0/u1/v0/v1 is not good for readability. uLow/uHigh/vLow/vHight? Also, we should have macros or functions like LOWORD / HIWORD provided by windows.h. > Source/WebCore/platform/Decimal.cpp:121 > + uint64_t w0 = u0 * v0; w0 should be productLower or something. > Source/WebCore/platform/Decimal.cpp:124 > + uint64_t tt = u1 * v0 + (w0 >> n2); > + uint64_t w1 = tt & mask; > + uint64_t w2 = tt >> n2; I have no good idea of better names for these variables. We need a comment if we don't have good variable names. > Source/WebCore/platform/Decimal.cpp:130 > +template<typename T> > +static T power(T const x, unsigned n) Please fold in this function to scaleUp(). This template is used only by scaleUp(), and code readers need to be afraid this template might overflow. If this is fold in scaleUp(), code readers can understand this is safe by ASSERT()s. > Source/WebCore/platform/Decimal.cpp:211 > +UInt128::UInt128(uint64_t low, uint64_t high) Please move this below the UInt128 class declaration. > Source/WebCore/platform/Decimal.cpp:216 > +UInt128& UInt128::operator/=(const uint32_t divisor) ditto. > Source/WebCore/platform/Decimal.cpp:237 > + const uint64_t work = (uint64_t(remainder) << 32) | dividend[i]; > + remainder = uint32_t(work % divisor); > + quotient[i] = uint32_t(work / divisor); > + } > + m_low = quotient[0] | (uint64_t(quotient[1]) << 32); > + m_high = quotient[2] | (uint64_t(quotient[3]) << 32); We should have a macro or a function like MAKEWORD provided by windows.h. > Source/WebCore/platform/Decimal.cpp:241 > +UInt128 UInt128::multiply(uint64_t u, uint64_t v) Please move this below the UInt128 class declaration. > Source/WebCore/platform/Decimal.cpp:413 > + // Make coefficient aligned. > + if (lhsExponent > rhsExponent) { > + const int numberOfLHSDigits = countDigits(lhsCoefficient); > + if (numberOfLHSDigits) { > + const int lhsShiftAmount = lhsExponent - rhsExponent; > + const int overflow = numberOfLHSDigits + lhsShiftAmount - Precision; > + if (overflow <= 0) > + lhsCoefficient = scaleUp(lhsCoefficient, lhsShiftAmount); > + else { > + lhsCoefficient = scaleUp(lhsCoefficient, lhsShiftAmount - overflow); > + rhsCoefficient = scaleDown(rhsCoefficient, overflow); > + resultExponent += overflow; > + } > + } > + > + } if (lhsExponent < rhsExponent) { > + const int numberOfRHSDigits = countDigits(rhsCoefficient); > + if (numberOfRHSDigits) { > + const int rhsShiftAmount = rhsExponent - lhsExponent; > + const int overflow = numberOfRHSDigits + rhsShiftAmount - Precision; > + if (overflow <= 0) > + rhsCoefficient = scaleUp(rhsCoefficient, rhsShiftAmount); > + else { > + rhsCoefficient = scaleUp(rhsCoefficient, rhsShiftAmount - overflow); > + lhsCoefficient = scaleDown(lhsCoefficient, overflow); > + resultExponent += overflow; > + } > + } > + } Please make a function for this operation. > Source/WebCore/platform/Decimal.cpp:415 > + uint64_t const result = lhsSign == rhsSign ? lhsCoefficient + rhsCoefficient : lhsCoefficient - rhsCoefficient; const uint64_t is common in WebKit. > Source/WebCore/platform/Decimal.cpp:420 > + return lhsSign == Negative && rhsSign == Positive && !result > + ? Decimal(Positive, resultExponent, 0) > + : int64_t(result) >= 0 > + ? Decimal(lhsSign, resultExponent, result) > + : Decimal(invertSign(lhsSign), resultExponent, -int64_t(result)); Nested ternary operators are confusing. Please use 'if' for the first condition. > Source/WebCore/platform/Decimal.cpp:512 > + while (work.high()) { > + work /= 10; > + ++resultExponent; > + } wrong indentation. > Source/WebCore/platform/Decimal.cpp:567 > + // 0/non-zero => 0 This comment looks not useful. > Source/WebCore/platform/Decimal.cpp:723 > + #define HandleCharAndBreak(ch1, next, ...) \ > + if (ch == ch1 || ch == ch1 - 'A' + 'a') { \ > + __VA_ARGS__; \ You don't use HandleCharAndBreak with __VA_ARGS__. > Source/WebCore/platform/Decimal.cpp:746 > + if (ch == '.') { > + state = StateDot; > + break; > + } should be HandleCharAndBreak('.', StateDot) ? > Source/WebCore/platform/Decimal.cpp:803 > + if (exponent > ExponentMax + Precision) > + return exponentSign == Negative ? zero(Positive) : infinity(sign); "0E+9999" is infinity? > Source/WebCore/platform/Decimal.cpp:823 > + if (ch == '0') { > + state = StateZero; > + break; > + } HandleCharAndBreak? > Source/WebCore/platform/Decimal.cpp:838 > + if (ch == '0') { > + state = StateZero; > + break; > + } HandleCharAndBreak ? > Source/WebCore/platform/Decimal.cpp:862 > + if (ch == '.') { > + state = StateDot; > + break; > + } HandleCharAndBreak ? > Source/WebCore/platform/Decimal.cpp:880 > + if (ch == '.') { > + state = StateDot; > + break; > + } HandleCharAndBreak? > Source/WebCore/platform/Decimal.h:51 > + // You should not use FormatClass. This is for implementation. > + enum FormatClass { Can you make this private? It's ok to make SpecialValueHandler and EncodedData friends of Decimal. > Source/WebCore/platform/Decimal.h:122 > + static Decimal fromString(const String&); You should add a comment about what is supported.

Created attachment 144700 [details] Patch 4

Comment on attachment 144700 [details] Patch 4 * Updated as reviewer's comment ** Introduce alignOperands function for operator+ and operator- ** Use HandleCharAndBreak for '.' ** Introduce HandleTwoCharsAndBreak for 'E' and 'e'.

Comment on attachment 144700 [details] Patch 4 Attachment 144700 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12846744

Comment on attachment 144700 [details] Patch 4 Attachment 144700 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12838852

Comment on attachment 144700 [details] Patch 4 Attachment 144700 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12843723

Comment on attachment 144700 [details] Patch 4 Attachment 144700 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12843726

Created attachment 144709 [details] Patch 5

Comment on attachment 144709 [details] Patch 5 * Fix typo... :< Could you review again?

Comment on attachment 144709 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=144709&action=review > Source/WebCore/platform/Decimal.cpp:99 > + static uint64_t makeUint64(uint32_t low, uint32_t high) { return low | (uint64_t(high) << 32); } should be renamed to makeU*I*nt64() for consistency. > Source/WebCore/platform/Decimal.cpp:107 > + > + One blank line is enough. > Source/WebCore/platform/Decimal.cpp:121 > + dividend[0] = uint32_t(m_low & uint32_t(-1)); > + dividend[1] = uint32_t(m_low >> 32); > + dividend[2] = uint32_t(m_high & uint32_t(-1)); > + dividend[3] = uint32_t(m_high >> 32); Please use highUInt32() and lowUInt32(). > Source/WebCore/platform/Decimal.cpp:126 > + const uint64_t work = (uint64_t(remainder) << 32) | dividend[i]; Use makeUInt64(). > Source/WebCore/platform/Decimal.cpp:186 > +SpecialValueHandler::SpecialValueHandler(const Decimal& lhs, const Decimal& rhs) Move the definitions of SpecialValueHandler functions to the line 80, just after the declaration of SpecialValueHandler and before the declaration of UInt128. > Source/WebCore/platform/Decimal.cpp:409 > + return lhsSign == Negative && rhsSign == Negative && !result > + ? Decimal(Positive, alignedOperands.exponent, 0) > + : int64_t(result) >= 0 > + ? Decimal(lhsSign, alignedOperands.exponent, result) > + : Decimal(invertSign(lhsSign), alignedOperands.exponent, -int64_t(result)); Don't use nested ternary operators. > Source/WebCore/platform/Decimal.cpp:627 > + return result.isZero() > + ? zero(Positive) > + : result.isFinite() > + ? result > + : result.isNegative() ? Decimal(-1) : Decimal(1); Please avoid nested ternary operators. > Source/WebCore/platform/Decimal.cpp:673 > + #define HandleCharAndBreak(expected, nextState) \ Preprocessor directives should start at the beginning of a line. > Source/WebCore/platform/Decimal.cpp:679 > + #define HandleTwoCharsAndBreak(expected1, expected2, nextState) \ ditto. > Source/WebCore/platform/Decimal.cpp:897 > + return sign() ? "-NaN" : "NaN"; We don't need to support -NaN, right? > Source/WebCore/platform/Decimal.h:76 > + // You should not use FormatClass. This is for implementation. We don't need this comment. > Source/WebCore/platform/Decimal.h:128 > + String toString() const; Please note this supports NaN and Infinity unlike fromString(). > Source/WebCore/platform/Decimal.h:136 > + // fromString supports following syntax EBNF: > + // number ::= sign? digit+ ('.' digit*) (exponent-marker sign? digit+)? > + // | sign? '.' digit+ (exponent-marker sign? digit+)? > + // sign ::= '+' | '-' > + // exponent-marker ::= 'e' | 'E' > + // digit ::= '0' | '1' | ... | '9' > + static Decimal fromString(const String&); Please note NaN and Infinity. > Source/WebKit/chromium/tests/DecimalTest.cpp:99 > + for (int i = 0; i < numberOfStepTimes; ++i) { > + value -= stepRange.step; > + value = stepRange.clampValue(value); > + } wrong indentation. > Source/WebKit/chromium/tests/DecimalTest.cpp:110 > + for (int i = 0; i < numberOfStepTimes; ++i) { > + value += stepRange.step; > + value = stepRange.clampValue(value); > + } wrong indentation > Source/WebKit/chromium/tests/DecimalTest.cpp:134 > + EXPECT_EQ(encode(1, -10, Positive), encode(1, -10, Negative).abs()); > +} Add tests for Infinity and NaN. > Source/WebKit/chromium/tests/DecimalTest.cpp:149 > + EXPECT_EQ(Decimal(-1), encode(19, -1, Negative).ceiling()); > +} Please add tests for - Infinity and NaN - large exponents - encode(std::numeric_limits<uint64_t>::max(), 0, Positive) > Source/WebKit/chromium/tests/DecimalTest.cpp:174 > + EXPECT_EQ(Decimal(-2), encode(19, -1, Negative).floor()); > +} Please add tests for - Infinity and NaN - large exponents > Source/WebKit/chromium/tests/DecimalTest.cpp:179 > + EXPECT_EQ(Decimal(1), Decimal(1)); should be encode(1, 0, Positive)? > Source/WebKit/chromium/tests/DecimalTest.cpp:184 > + EXPECT_EQ(encode(0x7FFFFFFF, 0, Positive), Decimal(INT_MAX)); > + EXPECT_EQ(encode(0x80000000u, 0, Negative), Decimal(INT_MIN)); should use std::numeric_limits<int32_t>::max() and mix() instead of INT_MAX and INT_MIN. > Source/WebKit/chromium/tests/DecimalTest.cpp:229 > + EXPECT_EQ(Decimal::nan(), fromString("INF")); > +} Add "Infinity", "NaN" > Source/WebKit/chromium/tests/DecimalTest.cpp:303 > +TEST_F(DecimalTest, OpAddBigE) BigE -> BigExponent > Source/WebKit/chromium/tests/DecimalTest.cpp:310 > +TEST_F(DecimalTest, OpAddSmallE) SmallE -> SmallExponent > Source/WebKit/chromium/tests/DecimalTest.cpp:340 > +TEST_F(DecimalTest, OpCompare) > +{ > + EXPECT_TRUE(Decimal(0) == Decimal(0)); Need more test cases. We know these operators were implemented by EncodedData equality and subtraction. But we need to test many cases here because someone will change the implementation in the future. > Source/WebKit/chromium/tests/DecimalTest.cpp:359 > + EXPECT_TRUE(Decimal(1) > Decimal::infinity(Negative)); > +} need more test cases. > Source/WebKit/chromium/tests/DecimalTest.cpp:361 > +TEST_F(DecimalTest, OpDiv) Div -> Division > Source/WebKit/chromium/tests/DecimalTest.cpp:372 > +TEST_F(DecimalTest, OpDivBigE) DivBigE -> DivisionBigExponent > Source/WebKit/chromium/tests/DecimalTest.cpp:378 > +TEST_F(DecimalTest, OpDivSmallE) DivSmallE -> DivisionSmallExponent > Source/WebKit/chromium/tests/DecimalTest.cpp:384 > +TEST_F(DecimalTest, OpDivSpecialValues) Div -> Division > Source/WebKit/chromium/tests/DecimalTest.cpp:424 > +TEST_F(DecimalTest, OpMul) Mul -> Multiplication > Source/WebKit/chromium/tests/DecimalTest.cpp:433 > +TEST_F(DecimalTest, OpMulBigE) MulBigE -> MultiplicationBigExponent > Source/WebKit/chromium/tests/DecimalTest.cpp:439 > +TEST_F(DecimalTest, OpMulSmallE) MulSmallE -> MultiplicationSmallExponent > Source/WebKit/chromium/tests/DecimalTest.cpp:445 > +TEST_F(DecimalTest, OpMulSpecialValues) Mul -> Multiplication > Source/WebKit/chromium/tests/DecimalTest.cpp:483 > + EXPECT_EQ(NaN, Ten * NaN); > +} Add NaN * Infinity > Source/WebKit/chromium/tests/DecimalTest.cpp:485 > +TEST_F(DecimalTest, OpSub) Sub -> Subtraction > Source/WebKit/chromium/tests/DecimalTest.cpp:496 > +TEST_F(DecimalTest, OpSubBigE) SubBigE -> SubtractionBigExponent > Source/WebKit/chromium/tests/DecimalTest.cpp:502 > +TEST_F(DecimalTest, OpSubSmallE) SubSmalleE SubtractionSmallE > Source/WebKit/chromium/tests/DecimalTest.cpp:508 > +TEST_F(DecimalTest, OpSubSpecialValues) Sub -> Subtraction > Source/WebKit/chromium/tests/DecimalTest.cpp:528 > + EXPECT_EQ(NaN, Ten - NaN); > +} Add NaN - Infinity and vice versa. > Source/WebKit/chromium/tests/DecimalTest.cpp:530 > +TEST_F(DecimalTest, Properties) Merge this to Predicates. > Source/WebKit/chromium/tests/DecimalTest.cpp:536 > +TEST_F(DecimalTest, PropertiesSpecial) This looks covered by PredicatesSpecialValues. > Source/WebKit/chromium/tests/DecimalTest.cpp:584 > + EXPECT_EQ(encode(3, -1, Negative), encode(36, -1, Positive).remainder(encode(13, -1, Positive))); > +} Add test cases for large exponent values. > Source/WebKit/chromium/tests/DecimalTest.cpp:603 > + EXPECT_EQ(Decimal(10), (fromString("10.2") / 1).round()); > +} Add test cases for large exponent values. > Source/WebKit/chromium/tests/DecimalTest.cpp:611 > +TEST_F(DecimalTest, ToStringSpecial) We should have ToString test for non-special values.

Created attachment 144775 [details] Patch 6

Comment on attachment 144775 [details] Patch 6 * Add tests for special values * Add tests for big/small exponent * Change Decimal::compareTo to handle NaN. * Change operator <, > to handle NaN comparison. Could you review again? Thanks in advance.

Comment on attachment 144775 [details] Patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=144775&action=review > Source/WebCore/ChangeLog:25 > + (WebCore): > + (DecimalPrivate): > + (SpecialValueHandler): > + (UInt128): > + (WebCore::DecimalPrivate::UInt128::high): > + (WebCore::DecimalPrivate::UInt128::low): This function list is not synchronized with the real one. > Source/WebCore/platform/Decimal.cpp:522 > + if (result.isNaN()) > + return false; wrong indentation > Source/WebCore/platform/Decimal.cpp:530 > + if (result.isNaN()) > + return false; ditto. > Source/WebCore/platform/Decimal.cpp:540 > + if (result.isNaN()) > + return false; ditto. > Source/WebCore/platform/Decimal.cpp:558 > + if (result.isNaN()) > + return false; ditto. > Source/WebCore/platform/Decimal.h:30 > + */ > +#ifndef Decimal_h Need a blank line between the copyright header and the first #ifndef. > Source/WebCore/platform/Decimal.h:130 > + // Note: toString method supports infinity and nan but formString not. formString -> fromString > Source/WebKit/chromium/tests/DecimalTest.cpp:612 > +TEST_F(DecimalTest, FromStringLikeNumber) > +{ > + EXPECT_EQ(Decimal::nan(), fromString(" 123 ")); > + EXPECT_EQ(Decimal::nan(), fromString("1,234")); > + EXPECT_EQ(Decimal::nan(), fromString("INF")); > +} Needs tests for "Infinity" and "NaN"

Comment on attachment 144775 [details] Patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=144775&action=review > Source/WebCore/platform/Decimal.cpp:143 > + static uint32_t highUInt32(uint64_t x) { return uint32_t(x >> 32); } > + static uint32_t lowUInt32(uint64_t x) { return uint32_t(x & ((uint64_t(1) << 32) - 1)); } Please use static_cast<uint32_t>() and static_cast<uint64_t>(1) or UIN64_C(1). > Source/WebCore/platform/Decimal.cpp:145 > + static uint64_t makeUInt64(uint32_t low, uint32_t high) { return low | (uint64_t(high) << 32); } static_cast<uint64_t>() > Source/WebCore/platform/Decimal.cpp:173 > + remainder = uint32_t(work % divisor); > + quotient[i] = uint32_t(work / divisor); static_cast<>() > Source/WebCore/platform/Decimal.cpp:373 > + return int64_t(result) >= 0 static_cast<>() > Source/WebCore/platform/Decimal.cpp:375 > + : Decimal(invertSign(lhsSign), alignedOperands.exponent, -int64_t(result)); static_cast<>() > Source/WebCore/platform/Decimal.cpp:411 > + return int64_t(result) >= 0 static_cast<>() > Source/WebCore/platform/Decimal.cpp:413 > + : Decimal(invertSign(lhsSign), alignedOperands.exponent, -int64_t(result)); static_cast<>() > Source/WebCore/platform/Decimal.cpp:548 > + if (result.isNaN()) > + return false; wrong indentation > Source/WebCore/platform/Decimal.cpp:961 > + int coefficientLength = int(digits.length()); static_cast<>()

Created attachment 144970 [details] Patch 7

Comment on attachment 144970 [details] Patch 7 Clearing flags on attachment: 144970 Committed r119062: <http://trac.webkit.org/changeset/119062>

All reviewed patches have been landed. Closing bug.

Reverted r119062 for reason: Broke build on Lion, SnowLoepard, Chromium Windows, and Chromium Linux 32 Committed r119067: <http://trac.webkit.org/changeset/119067>

Comment on attachment 144970 [details] Patch 7 View in context: https://bugs.webkit.org/attachment.cgi?id=144970&action=review > Source/WebCore/WebCore.gypi:276 > + 'platform/Decimal.h' > 'platform/DragData.h', Need a comma > Source/WebCore/platform/Decimal.cpp:281 > +Decimal::Decimal(int32_t i32) > + : m_data(i32 < 0 ? Negative : Positive, 0, i32 < 0 ? static_cast<uint32_t>(-i32) : i32) We need a test for Decimal(std::numeric_limits<int32_t>::min()). > Source/WebKit/chromium/tests/DecimalTest.cpp:165 > + EXPECT_EQ(encode(1000000000000000, 35, Positive), encode(1, 50, Positive) + Decimal(1)); > + EXPECT_EQ(encode(1000000000000000, 35, Positive), Decimal(1) + encode(1, 50, Positive)); > + EXPECT_EQ(encode(10000000001, 0, Positive), encode(1, 10, Positive) + Decimal(1)); > + EXPECT_EQ(encode(10000000001, 0, Positive), Decimal(1) + encode(1, 10, Positive)); Need to use UINT64_C(). > Source/WebKit/chromium/tests/DecimalTest.cpp:454 > + EXPECT_EQ(encode(3333333333333333, -16, Positive), Decimal(1) / Decimal(3)); > + EXPECT_EQ(encode(12345678901234, -1, Positive), encode(12345678901234, 0, Positive) / Decimal(10)); ditto. > Source/WebKit/chromium/tests/DecimalTest.cpp:899 > + EXPECT_EQ(encode(1000000000000000, 35, Positive), encode(1, 50, Positive) - Decimal(1)); > + EXPECT_EQ(encode(1000000000000000, 35, Negative), Decimal(1) - encode(1, 50, Positive)); ditto.

(In reply to comment #27) > > Source/WebCore/platform/Decimal.cpp:281 > > +Decimal::Decimal(int32_t i32) > > + : m_data(i32 < 0 ? Negative : Positive, 0, i32 < 0 ? static_cast<uint32_t>(-i32) : i32) > > We need a test for Decimal(std::numeric_limits<int32_t>::min()). We've had it.

Created attachment 145009 [details] Patch 8

Comment on attachment 145009 [details] Patch 8 View in context: https://bugs.webkit.org/attachment.cgi?id=145009&action=review > Source/WebCore/platform/Decimal.cpp:281 > + : m_data(i32 < 0 ? Negative : Positive, 0, static_cast<uint32_t>(i32 < 0 ? -i32 : i32)) I think -i32 for std::numeric_limits<int32_t>::min() is a undefined behavior.

(In reply to comment #30) > (From update of attachment 145009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145009&action=review > > > Source/WebCore/platform/Decimal.cpp:281 > > + : m_data(i32 < 0 ? Negative : Positive, 0, static_cast<uint32_t>(i32 < 0 ? -i32 : i32)) > > I think -i32 for std::numeric_limits<int32_t>::min() is a undefined behavior. You're right. I changed to: Decimal::Decimal(int32_t i32) : m_data(i32 < 0 ? Negative : Positive, 0, i32 < 0 ? (UINT64_C(1) << 32) - static_cast<uint32_t>(i32) : static_cast<uint64_t>(i32))

(In reply to comment #31) > (In reply to comment #30) > > (From update of attachment 145009 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=145009&action=review > > > > > Source/WebCore/platform/Decimal.cpp:281 > > > + : m_data(i32 < 0 ? Negative : Positive, 0, static_cast<uint32_t>(i32 < 0 ? -i32 : i32)) > > > > I think -i32 for std::numeric_limits<int32_t>::min() is a undefined behavior. > > You're right. > > I changed to: > > Decimal::Decimal(int32_t i32) > : m_data(i32 < 0 ? Negative : Positive, 0, i32 < 0 ? (UINT64_C(1) << 32) - static_cast<uint32_t>(i32) : static_cast<uint64_t>(i32)) 2^32 - uint(x) doesn't work, if CPU uses one's complement. Let's make 0x80000000 as special case: Decimal::Decimal(int32_t i32) : m_data(i32 < 0 ? Negative : Positive, 0, static_cast<uint32_t>(static_cast<uint32_t>(i32) == 0x80000000u || i32 >=0 0 ? i32 : -i32))

Comment on attachment 145009 [details] Patch 8 Clearing flags on attachment: 145009 Committed r119073: <http://trac.webkit.org/changeset/119073>

All reviewed patches have been landed. Closing bug.

Oops, it was already landed. Please make another patch. > > Decimal::Decimal(int32_t i32) > > : m_data(i32 < 0 ? Negative : Positive, 0, i32 < 0 ? (UINT64_C(1) << 32) - static_cast<uint32_t>(i32) : static_cast<uint64_t>(i32)) > > 2^32 - uint(x) doesn't work, if CPU uses one's complement. > > Let's make 0x80000000 as special case: > > Decimal::Decimal(int32_t i32) > : m_data(i32 < 0 ? Negative : Positive, 0, static_cast<uint32_t>(static_cast<uint32_t>(i32) == 0x80000000u || i32 >=0 0 ? i32 : -i32)) Looks ugly. How about i32 < 0 ? static_cast<uint64_t>(-static_cast<int64_t>(i32)) : static_cast<uint64_t>(i32) ?

(In reply to comment #35) > How about > i32 < 0 ? static_cast<uint64_t>(-static_cast<int64_t>(i32)) : static_cast<uint64_t>(i32) It is better than having special case. I filed bug 87941 for this. Thanks!

It's very unfortunate that we have WebCore/platform/Decimal.h and wtf/DecimalNumber.h now. The wtf/*Number*.h classes are intended for xx <-> String conversions, while your Decimal class does much more. Still I see no reason why this code should live in platform/ rather than WTF. Also do you real need all of this functionality that you exposed in Decimal? While the code reads nicely, its a huge pile of new complex code and I don't yet see users of it, nor can I imagine that you need all of the code you've added :-)

(In reply to comment #37) > It's very unfortunate that we have WebCore/platform/Decimal.h and wtf/DecimalNumber.h now. > The wtf/*Number*.h classes are intended for xx <-> String conversions, while your Decimal class does much more. > > Still I see no reason why this code should live in platform/ rather than WTF. > Also do you real need all of this functionality that you exposed in Decimal? > > While the code reads nicely, its a huge pile of new complex code and I don't yet see users of it, nor can I imagine that you need all of the code you've added :-) Yes, we need to have decimal arithmetic for number input type and range input type to avoid rounding error in stepping numbers which aren't represent correctly in radix two floating pointer number. For example, in HTML <input type=range step=0.1 value=0 id=foo> Script $("foo").stepUp(6) or move slider 6 times . We would like to have $("foo").value="0.6". Bug 80009 has more sample for double floating pointer number rounding issue.

(In reply to comment #33) > (From update of attachment 145009 [details]) > Clearing flags on attachment: 145009 > > Committed r119073: <http://trac.webkit.org/changeset/119073> This broke the Lion build (fixed in http://trac.webkit.org/changeset/119109).