Bug 96030 - Add StringBuilder::appendNumber() and use it
Summary: Add StringBuilder::appendNumber() and use it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on: 95940
Blocks:
  Show dependency tree / graph
 
Reported: 2012-09-06 15:00 PDT by Patrick R. Gansterer
Modified: 2012-09-09 21:19 PDT (History)
11 users (show)

See Also:

Attachments
Patch (20.96 KB, patch)
2012-09-06 15:07 PDT, Patrick R. Gansterer 		no flags Details | Formatted Diff | Diff
Patch (23.70 KB, patch)
2012-09-09 15:06 PDT, Patrick R. Gansterer 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.