Bug 8009 - Drag DeprecatedString 3 steps closer to its grave...
Summary: Drag DeprecatedString 3 steps closer to its grave...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P4 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-03-27 02:55 PST by Eric Seidel (no email)
Modified: 2006-03-28 11:04 PST (History)
1 user (show)

See Also:


Attachments
Replace a bunch more DeprecatedString with String (88.81 KB, patch)
2006-03-27 02:56 PST, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff
Bigger patch, ultra slow however. (142.72 KB, patch)
2006-03-27 20:01 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Bigger patch, ultra slow however. (109.87 KB, patch)
2006-03-27 20:02 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Addresses darin's comments. (74.81 KB, patch)
2006-03-27 23:31 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Addresses darin's comments. (76.02 KB, patch)
2006-03-27 23:39 PST, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-03-27 02:55:29 PST
This patch replaces a whole bunch more DeprecatedString with String.  It does this by adding String::number() and String::sprintf().

This also fixes a windows bug found in DeprecatedString::sprintf when moving to String::sprintf()
Comment 1 Eric Seidel (no email) 2006-03-27 02:56:21 PST
Created attachment 7329 [details]
Replace a bunch more DeprecatedString with String
Comment 2 Darin Adler 2006-03-27 17:19:02 PST
Comment on attachment 7329 [details]
Replace a bunch more DeprecatedString with String

Why the limit of 2048 on buffer size of sprintf on WIN_OS?

+    Vector<char> buffer;

Should use Vector<char, 256> for better performance here.

I think that String's sprintf should return a String instead of being a function that mutates an existing string. Maybe it should be a free function called StringPrintF or StringFormat?

Otherwise, r=me.
Comment 3 Eric Seidel (no email) 2006-03-27 20:01:28 PST
Created attachment 7343 [details]
Bigger patch, ultra slow however.

So I ended up making a couple more mods to my local tree, which I now regret.  Those mods are in this patch.  The previous patch had two crashers, which I later discovered and have since fixed.  This patch however changes QTextStream over to use String instead of DeprecatedString, and in the process unveils how absolutely awful StringImpl::append() is.  StringImpl::append() allocates a new buffer of the appropriate size eery time.  DeprecatedString::append() woudl allocate a buffer which was sometimes larger than necessary to amortize the cost of the malloc over several append calls.  String is going to need to have something similar.  I expect that Vector already has some of this logic.  I wonder how well it could work to have StringImpl just hold a Vector<QChar>.
Comment 4 Eric Seidel (no email) 2006-03-27 20:02:51 PST
Created attachment 7344 [details]
Bigger patch, ultra slow however.
Comment 5 Eric Seidel (no email) 2006-03-27 23:31:23 PST
Created attachment 7350 [details]
Addresses darin's comments.

This patch fixes a couple crashers in the first patch, and addresses darin's comments.
Comment 6 Eric Seidel (no email) 2006-03-27 23:34:07 PST
(In reply to comment #2)
> (From update of attachment 7329 [details] [edit])
> Why the limit of 2048 on buffer size of sprintf on WIN_OS?

As far as I can tell I need an arbitrary limit to prevent infinite looping.  vsnprintf always returns -1, no matter what the error.
 
> +    Vector<char> buffer;
> 
> Should use Vector<char, 256> for better performance here.

I don't see that to be true.  Vector<char> makes an empty buffer.  The very next line I call buffer.resize(result + 1); so I don't see why I would want to allocate a 256 char buffer just to throw it away.  But maybe I'm missing something simple here.

> I think that String's sprintf should return a String instead of being a
> function that mutates an existing string. Maybe it should be a free function
> called StringPrintF or StringFormat?

I changed it to be a static function.  That seems to work just as well.
Comment 7 Eric Seidel (no email) 2006-03-27 23:39:25 PST
Created attachment 7351 [details]
Addresses darin's comments.

Wrong patch before.
Comment 8 Darin Adler 2006-03-28 10:16:22 PST
Comment on attachment 7351 [details]
Addresses darin's comments.

r=me