Bug 109709 – wtf/dtoa/* uses a confusing name to reference its buffers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

wtf/dota/* is copying Vectors all over the place

I suspect this code was ported from stl.  In WTF, Vector<T> as an argument will copy the whole contents of the vector, so we normally use const Vector<T>&

grep -r "(Vector" * | grep -v '&'
wtf/dtoa/bignum.cc:    static uint64_t ReadUInt64(Vector<const char> buffer,
wtf/dtoa/bignum.cc:    void Bignum::AssignDecimalString(Vector<const char> value) {
wtf/dtoa/bignum.cc:    void Bignum::AssignHexString(Vector<const char> value) {
wtf/dtoa/bignum.h:        void AssignDecimalString(Vector<const char> value);
wtf/dtoa/bignum.h:        void AssignHexString(Vector<const char> value);
wtf/dtoa/double-conversion.cc:        double converted = Strtod(Vector<const char>(buffer, buffer_pos), exponent);
wtf/dtoa/fast-dtoa.cc:    static bool RoundWeed(Vector<char> buffer,
wtf/dtoa/fast-dtoa.cc:    static bool RoundWeedCounted(Vector<char> buffer,
wtf/dtoa/fixed-dtoa.cc:    static void RoundUp(Vector<char> buffer, int* length, int* decimal_point) {
wtf/dtoa/fixed-dtoa.cc:    static void TrimZeros(Vector<char> buffer, int* length, int* decimal_point) {
wtf/dtoa/strtod.cc:    static Vector<const char> TrimLeadingZeros(Vector<const char> buffer) {
wtf/dtoa/strtod.cc:    static Vector<const char> TrimTrailingZeros(Vector<const char> buffer) {
wtf/dtoa/strtod.cc:    static void TrimToMaxSignificantDigits(Vector<const char> buffer,
wtf/dtoa/strtod.cc:    static uint64_t ReadUint64(Vector<const char> buffer,
wtf/dtoa/strtod.cc:    static void ReadDiyFp(Vector<const char> buffer,
wtf/dtoa/strtod.cc:    static bool DoubleStrtod(Vector<const char> trimmed,
wtf/dtoa/strtod.cc:    static bool DiyFpStrtod(Vector<const char> buffer,
wtf/dtoa/strtod.cc:    static double BignumStrtod(Vector<const char> buffer,
wtf/dtoa/strtod.cc:    double Strtod(Vector<const char> buffer, int exponent) {
wtf/dtoa/strtod.cc:            return Strtod(Vector<const char>(significant_buffer,
wtf/dtoa/strtod.h:    double Strtod(Vector<const char> buffer, int exponent);

[Benjamin Poulain]

Interesting find!

Do you want me to take care of it or do you already have a patch?

[Eric Seidel (no email)]

I have no plans to write a patch anytime soon, sadly.  Must... finish... parser...

[Eric Seidel (no email)]

Aka, it's all yours if you want it.

I would expect this to show up in Dromeo numbers. :)

[Benjamin Poulain]

Created attachment 189627 [details]
Patch

[WebKit Review Bot]

Attachment 189627 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/dtoa/bignum-dtoa.cc', u'Source/WTF/wtf/dtoa/bignum-dtoa.h', u'Source/WTF/wtf/dtoa/bignum.cc', u'Source/WTF/wtf/dtoa/bignum.h', u'Source/WTF/wtf/dtoa/double-conversion.cc', u'Source/WTF/wtf/dtoa/fast-dtoa.cc', u'Source/WTF/wtf/dtoa/fast-dtoa.h', u'Source/WTF/wtf/dtoa/fixed-dtoa.cc', u'Source/WTF/wtf/dtoa/fixed-dtoa.h', u'Source/WTF/wtf/dtoa/strtod.cc', u'Source/WTF/wtf/dtoa/strtod.h', u'Source/WTF/wtf/dtoa/utils.h']" exit_code: 1
Source/WTF/wtf/dtoa/fast-dtoa.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WTF/wtf/dtoa/fast-dtoa.h:80:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/dtoa/bignum.h:133:  bigits_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/dtoa/bignum-dtoa.h:80:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/dtoa/bignum-dtoa.h:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WTF/wtf/dtoa/utils.h:144:  Missing space inside { }.  [whitespace/braces] [5]
Source/WTF/wtf/dtoa/utils.h:144:  Use 0 instead of NULL.  [readability/null] [5]
Source/WTF/wtf/dtoa/utils.h:145:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WTF/wtf/dtoa/utils.h:151:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WTF/wtf/dtoa/utils.h:258:  buffer_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/dtoa/strtod.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/dtoa/strtod.h:39:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/dtoa/fixed-dtoa.h:54:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/dtoa/fixed-dtoa.h:54:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 189627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189627&action=review

> Source/WTF/wtf/dtoa/utils.h:140
> -    // This is a simplified version of V8's Vector class.
> +    // This is a simplified version of V8's BufferReference class.

Probably want to fix this comment now.

[Eric Seidel (no email)]

Did we copy this code from v8?  Do we intend to maintain a copy?  Do we want it to diverge?

[Benjamin Poulain]

> > Source/WTF/wtf/dtoa/utils.h:140
> > -    // This is a simplified version of V8's Vector class.
> > +    // This is a simplified version of V8's BufferReference class.
> 
> Probably want to fix this comment now.

Oops, will do.
I should probably have read the result of my FindAndReplace :)

> Did we copy this code from v8?  Do we intend to maintain a copy?  Do we want it to diverge?

We already improved the V8 code quite a bit since it was imported.

[Darin Adler]

Comment on attachment 189627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189627&action=review

>> Source/WTF/wtf/dtoa/utils.h:140
>> -    // This is a simplified version of V8's Vector class.
>> +    // This is a simplified version of V8's BufferReference class.
> 
> Probably want to fix this comment now.

Let me amplify what Eric said. This comment makes no sense like this. Please fix it before landing.

[Darin Adler]

Comment on attachment 189627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189627&action=review

>>> Source/WTF/wtf/dtoa/utils.h:140
>>> +    // This is a simplified version of V8's BufferReference class.
>> 
>> Probably want to fix this comment now.
> 
> Let me amplify what Eric said. This comment makes no sense like this. Please fix it before landing.

Oops, I now see Ben replying, so he didn’t need me to say this again! Sorry.

[Benjamin Poulain]

Committed r147976: <http://trac.webkit.org/changeset/147976>