Bug 175988 - Add String::format variant that takes va_args
Summary: Add String::format variant that takes va_args
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-25 10:32 PDT by Eric Carlson
Modified: 2017-08-28 14:01 PDT (History)
10 users (show)

See Also:


Attachments
Proposed patch. (6.51 KB, patch)
2017-08-25 11:37 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Address post-review feedback. (1.37 KB, patch)
2017-08-28 08:41 PDT, Eric Carlson
eric.carlson: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-08-25 10:32:58 PDT
Add String::format variant that takes va_args so it can be used from a variadic function.
Comment 1 Eric Carlson 2017-08-25 11:37:08 PDT
Created attachment 319091 [details]
Proposed patch.
Comment 2 Radar WebKit Bug Importer 2017-08-25 11:37:55 PDT
<rdar://problem/34085521>
Comment 3 WebKit Commit Bot 2017-08-25 13:24:01 PDT
Comment on attachment 319091 [details]
Proposed patch.

Clearing flags on attachment: 319091

Committed r221203: <http://trac.webkit.org/changeset/221203>
Comment 4 WebKit Commit Bot 2017-08-25 13:24:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Geoffrey Garen 2017-08-25 15:23:04 PDT
Comment on attachment 319091 [details]
Proposed patch.

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

> Source/WTF/wtf/text/WTFString.cpp:471
> +    va_copy(argsCopy, args);

This function calls "va_end(args);" in two places. It also passes "args" to some callees. I think all uses of "args" need to become uses of "argsCopy", no?
Comment 6 Eric Carlson 2017-08-25 16:45:02 PDT
(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 319091 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319091&action=review
> 
> > Source/WTF/wtf/text/WTFString.cpp:471
> > +    va_copy(argsCopy, args);
> 
> This function calls "va_end(args);" in two places. It also passes "args" to
> some callees. I think all uses of "args" need to become uses of "argsCopy",
> no?

Oops, "va_end(args)" is definitely wrong, but AFAIK the other uses of "args" is correct.
Comment 7 Darin Adler 2017-08-25 20:51:06 PDT
We should stop using String::format.
Comment 8 Darin Adler 2017-08-25 20:54:48 PDT
And instead use something that is type safe. Like use makeString.
Comment 9 Eric Carlson 2017-08-28 08:41:59 PDT
Created attachment 319179 [details]
Address post-review feedback.
Comment 10 Eric Carlson 2017-08-28 14:01:55 PDT
(In reply to Darin Adler from comment #8)
> And instead use something that is type safe. Like use makeString.

Filed bug 176035 for this suggestion.