[webkit-reviews] review denied: [Bug 87360] [Platform] Introduce Decimal class for Number/Range input type. : [Attachment 144276] Patch 3
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 29 00:02: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 144276: Patch 3
https://bugs.webkit.org/attachment.cgi?id=144276&action=review
------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.
More information about the webkit-reviews
mailing list