Bug 8009 – Drag DeprecatedString 3 steps closer to its grave...

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Eric Seidel (no email)]

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()

[Eric Seidel (no email)]

Created attachment 7329 [details]
Replace a bunch more DeprecatedString with String

[Darin Adler]

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.

[Eric Seidel (no email)]

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>.

[Eric Seidel (no email)]

Created attachment 7344 [details]
Bigger patch, ultra slow however.

[Eric Seidel (no email)]

Created attachment 7350 [details]
Addresses darin's comments.

This patch fixes a couple crashers in the first patch, and addresses darin's comments.

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

Created attachment 7351 [details]
Addresses darin's comments.

Wrong patch before.

[Darin Adler]

Comment on attachment 7351 [details]
Addresses darin's comments.

r=me