Bug 96030

Summary: Add StringBuilder::appendNumber() and use it
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: Web Template FrameworkAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, cmarcelo, eric, haraken, japhet, macpherson, menard, mifenton, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 95940    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Patrick R. Gansterer 2012-09-06 15:00:10 PDT
Add StringBuilder::appendNumber() and use it
Comment 1 Patrick R. Gansterer 2012-09-06 15:07:19 PDT
Created attachment 162597 [details]
Patch
Comment 2 Benjamin Poulain 2012-09-06 15:13:46 PDT
Nice!
Comment 3 Eric Seidel (no email) 2012-09-09 14:32:17 PDT
Comment on attachment 162597 [details]
Patch

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

Faster, or just cleaner?

> Source/JavaScriptCore/API/JSContextRef.cpp:210
> +        builder.append(':');

We really need to rename this appendChar or overload appendLiteral(char) instead so it's easier to tell that it's the "fast" version.
Comment 4 Build Bot 2012-09-09 14:32:49 PDT
Comment on attachment 162597 [details]
Patch

Attachment 162597 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13796642
Comment 5 Patrick R. Gansterer 2012-09-09 14:40:54 PDT
(In reply to comment #3)
> (From update of attachment 162597 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162597&action=review
> 
> Faster, or just cleaner?

Since it does not create a StringImpl for every number and appends the content of a character buffer directly it is faster too.

> > Source/JavaScriptCore/API/JSContextRef.cpp:210
> > +        builder.append(':');
> 
> We really need to rename this appendChar or overload appendLiteral(char) instead so it's easier to tell that it's the "fast" version.

I can do this in a follow up patch.
Comment 6 Patrick R. Gansterer 2012-09-09 15:06:57 PDT
Created attachment 163017 [details]
Patch
Comment 7 Eric Seidel (no email) 2012-09-09 15:11:30 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > > Source/JavaScriptCore/API/JSContextRef.cpp:210
> > > +        builder.append(':');
> > 
> > We really need to rename this appendChar or overload appendLiteral(char) instead so it's easier to tell that it's the "fast" version.
> 
> I can do this in a follow up patch.

yes. :)  I certainly wan't expecting such in this patch.   THanks!
Comment 8 Eric Seidel (no email) 2012-09-09 15:11:47 PDT
Comment on attachment 163017 [details]
Patch

LGTM if the EWSes think so.
Comment 9 WebKit Review Bot 2012-09-09 21:19:12 PDT
Comment on attachment 163017 [details]
Patch

Clearing flags on attachment: 163017

Committed r128014: <http://trac.webkit.org/changeset/128014>
Comment 10 WebKit Review Bot 2012-09-09 21:19:16 PDT
All reviewed patches have been landed.  Closing bug.