[webkit-reviews] review requested: [Bug 31330] Expose dtoa() for ECMA-262 : [Attachment 43340] Proposed patch (rev.4)

[bugzilla-daemon at webkit.org]

TAMURA, Kent <tkent at chromium.org> has asked  for review:
Bug 31330: Expose dtoa() for ECMA-262
https://bugs.webkit.org/show_bug.cgi?id=31330

Attachment 43340: Proposed patch (rev.4)
https://bugs.webkit.org/attachment.cgi?id=43340&action=review

------- Additional Comments from TAMURA, Kent <tkent at chromium.org>
Thank you for reviewing.  I have updated the patch.

(In reply to comment #7)
> > +	 unsigned len = ecma262dtoa(d, buf);
> > +	 buf[len] = 0;
> >	 return UString(buf);
> >  }
> 
> We could make this even more efficient by making a new UString constructor
that
> takes a const char* and a length to avoid the unnecessary strlen. That's not
> strictly necessary, but sure would be nice.

I added a UString(const char*, unsigned) and createRep(ditto).

> > +//#include <wtf/MathExtras.h>
> >  #include <wtf/Vector.h>
> >  #include <wtf/Threading.h>
> 
> Why the added-but-commented-out include?

Oops, removed.

> > +static ALWAYS_INLINE char* append(char* next, const char* src, unsigned
size)
> > +static ALWAYS_INLINE char* append(char* next, const char* src)
> 
> I would have made the "next" argument be a char*& rather than using a return
> value.

done.

> Why the casts to unsigned char? Those are not at all helpful.

Removed.  It was meaningful when the destination was UChar*.

> > +	 // dtoa() for ECMA-262 'ToString Applied to the Number Type.'
> > +	 // The buffer pointed by buf must have at least 80 elements.
> 
> I'd like to see a constant with that value 80. It's not good to have a
comment
> in one file and then the number typed in some other source file. Maybe
> something like this?
> 
>     typedef char dtoaECMA262Buffer[80];

Introduced DtoaBuffer, which is for both of dtoa() and this.

> > +	 // The return value is the length of the resultant string in buf.
> > +	 // The resultant string isn't terminated by 0.
> > +	 unsigned ecma262dtoa(double d, char* buf);
> 
> I don't think it's great to name the function dtoa when it returns a length.
> Maybe the length could be an out argument instead. Or somehow we could name
the
> function in a way that makes it clear its return value is the length of the
> string.
> 
> It might be better to give this a name with a nicer format.I'm not sure
having
> ECMA262 in the name is good. The colloquial name for this is JavaScript, so
> maybe this super-long name would be OK, or maybe some shorter variant.
> 
>     doubleToStringInJavaScriptFormatReturningLength
>
> The argument name "d" doesn't add anything here, so I'd suggest leaving it
out.
> The argument name "buf" could instead be a word, "buffer".

I changed it to:
  void doubleToStringInJavaScriptFormat(double, DtoaBuffer buffer, unsigned*
resultLength);