[webkit-reviews] review denied: [Bug 87360] [Platform] Introduce Decimal class for Number/Range input type. : [Attachment 144709] Patch 5
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 30 00:36:04 PDT 2012
Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 87360: [Platform] Introduce Decimal class for Number/Range input type.
https://bugs.webkit.org/show_bug.cgi?id=87360
Attachment 144709: Patch 5
https://bugs.webkit.org/attachment.cgi?id=144709&action=review
------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.
More information about the webkit-reviews
mailing list