[webkit-reviews] review denied: [Bug 87360] [Platform] Introduce Decimal class for Number/Range input type. : [Attachment 143942] Patch 2

[bugzilla-daemon at webkit.org]

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 143942: Patch 2
https://bugs.webkit.org/attachment.cgi?id=143942&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.